4

I get the "Debug assertion failed" error when my program ends. I've been trying to fix it for a long time and just can't find the cause. Even my prof in uni said he sees nothing wrong. So you are my last hope, stackoverllow. Please help.

The program finds the intersection of two lists and then checks if the third list is a subset of the intersection.

The screenshot of the error:

Here

The code:

list.h:

#ifndef __LIST_H_INCLUDED__
#define __LIST_H_INCLUDED__
#include <string>
#include <iostream>
#include <fstream>

struct node
{
    int value;
    node *next;
};

class list
{
    node* head;
public:
    list();
    ~list();
    void AddNodes(std::istream &input);
    void PrintList(std::ostream &output = std::cout);
    void AddOneNode(int AddVal);
    node* RetHead();
    list* Intersection(list* list2);
    bool IsPresent(int val);
    bool Subset(list subset);
 };

 #endif

list.cpp:

#include "stdafx.h"
#include "list.h"
#include <iostream>
#include <fstream>


list::list()
{
    head=NULL;
}

list::~list()
{

    node* current = head;
    while( current != 0 ) 
    {
        node* next = current->next;
        delete current;
        current = next;
    }
    head = 0;

}

void list::AddNodes(std::istream &input)
{
    int InVal;
    while(input>>InVal)
        AddOneNode(InVal);
}

void list::AddOneNode(int AddVal)
{
    node *NewNode= new node;
    NewNode->value=AddVal;
    NewNode->next=NULL;
    if(!head)
        head=NewNode;
    else
        {
            node *temp=head;
            while(temp->next)
                temp=temp->next;
            temp->next=NewNode;
        }
}

void list::PrintList(std::ostream &output)
{
    node *temp=head;
    while(temp)
    {
        output<<temp->value<<std::endl;
        temp=temp->next;

    }
}

list* list::Intersection(list *list2)
{
    list* result=new list;
    node* temp1=head;
    while(temp1)
    {
        if(list2->IsPresent(temp1->value))
            result->AddOneNode(temp1->value);
        temp1=temp1->next;

    }
    return result;
}

bool list::IsPresent(int val)
{
    node *temp=head;
    while(temp)
    {
        if(temp->value==val)
            return true;
        temp=temp->next;
    }
    return false;
}


bool list::Subset(list subset) // head=set
{
    bool flag;
    node* tempset=head;
    node* tempsub=subset.RetHead();
    while(tempset)
    {
        if (tempsub->value==tempset->value)
        {
            flag=true;
            break;
        }
        tempset=tempset->next;
    }
    if (!tempset)
        return false;
    while(tempsub)
    {
        tempsub=tempsub->next;
        if(!tempsub)
            return true;
        while(tempsub->value!=tempset->value&&tempset)
            tempset=tempset->next;
        if(!tempset)
            return false;
    }
    return flag;
}

node* list::RetHead()
{
    return head;
}

main.cpp:

#include "stdafx.h"
#include "list.h"
#include <Windows.h>
#include <fstream>

list Cross (list list1, list list2);
bool Subset (list set, list subset);

int main()
{
    setlocale (LC_ALL, "Russian");
    list l1,l2,l3;
    std::ifstream fl1 ("l1.txt");
    std::ifstream fl2 ("l2.txt");
    std::ifstream fl3 ("l3.txt");
    l1.AddNodes(fl1);
    std::cout<<"List 1:"<<std::endl;
    l1.PrintList();
    std::cout<<std::endl;
    l2.AddNodes(fl2);
    std::cout<<"List 2:"<<std::endl;
    l2.PrintList();
    std::cout<<std::endl;
    l3.AddNodes(fl3);
    std::cout<<"List 3:"<<std::endl;
    l3.PrintList();
    std::cout<<"Intersection of list 1 and list 2"<<std::endl;
    list *intersec=l1.Intersection(&l2);
    intersec->PrintList();
    std::cout<<std::endl;
    if(intersec->Subset(l3))
        std::cout<<"Third set is a subset of the intersection"<<std::endl;
    else
        std::cout<<"Third set is not a subset of the intersection"<<std::endl;
    system("pause");
    return 0;
}
Ivan
  • 16,536
  • 6
  • 23
  • 36
  • 1
    Your include guard uses a [reserved identifier](http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier). You also aren't following the rule of three (really, don't use owning raw pointers). – chris Oct 26 '14 at 16:00
  • What is your input data? – Captain Obvlious Oct 26 '14 at 16:01
  • You are most likely "double-freeing" something. – Mats Petersson Oct 26 '14 at 16:02
  • @chris while true, I dont think it is the cause here, because if you make unique enough name, chances of conflicts are pretty much 0 – Creris Oct 26 '14 at 16:03
  • 1
    @Creris, I don't think it's the cause, but it's certainly not a good idea. – chris Oct 26 '14 at 16:04
  • @chris not my question but still Im curious, what is better naming for header guards? I personally do one underscore and allcaps identifiers, but I think these are also reserved. I know this is kind of bad to ask on someone else's question, but this is pretty offtopic question overall – Creris Oct 26 '14 at 16:05
  • @CaptainObvlious Well, it's just int sets, l1>l2>l3. l1 is numbers from 1 to 9, l2 is 3 4 5 6 and l3 is 4 5. – Ivan Oct 26 '14 at 16:06
  • @CaptainObvlious Yeah, I wish we could get a better prof and i'm pretty sure he told us nothing about the rule of 3. But it seems like learning programming is all about self-education anyway. – Ivan Oct 26 '14 at 16:20
  • @Creris, I stick with `PROJECT_FILE_H`, but to be honest, I usually just use `#pragma once` instead. Not standard, but everything I could hope to target has it, and a bit better in the sense that it's an extension in an area explicitly allowed (custom pragmas). – chris Oct 26 '14 at 16:32

1 Answers1

9

The problem is that the function list::Subset(list subset) takes its argument by value causing a copy of the list to be made. Since you did not follow the Rule of Three (as noted in Chris' comment) a shallow copy is made. This means that two instance of list "own" the pointers. When the Subset function returns the copy goes out of scope causing the nodes to be deleted. When the program exits the original copy of the list goes out of scope and it attempts to delete the same nodes again causing the assertion.

You can get around this by taking the argument by reference instead of by value. Change

class list
{
    // ... snip ...
    bool Subset(list subset);
    // ... snip ...
};

to

class list
{
    // ... snip ...
    bool Subset(list& subset);
    // ... snip ...
};

and

bool list::Subset(list subset)
{
    // ... snip ...
}

to

bool list::Subset(list& subset)
{
    // ... snip ...
}

Some other suggestions:

  1. Either implement a proper copy constructor or declare one and make it private to prevent copies from being made
  2. Learn const correctness. Since Subset does not modify the contents of the list passed to it you can declare it bool list::Subset(const list&) const instead. This will require list::RetHead() to be declared const as well.
  3. bool flag in list::Subset is not initialized meaning that any value can be returned if your logic is not correct.
Captain Obvlious
  • 19,754
  • 5
  • 44
  • 74