-2

my destructor:

~list() {
    for (node *p; !isEmpty();) 
    {
        p = head->next;
        delete head;
        head = p;
    }
}

My function append:

void append( list L2)
{

    if (head == 0 && L2.head == 0)
    {
        cout << "Error List Empty" << endl;



    }
    else if (isEmpty())
    {
            L2.printAll();
    }
    else if (L2.isEmpty())
    {
            printAll();  
    }


    else
    {
        tail->next = L2.head;
        tail = L2.tail;
        L2.head = L2.tail = 0;
    }
}

The code stops when the destructor tries to run.

My main:

int main()
{
list l1;
l1.addToHead(1);
l1.addToHead(2);

list l2;
l2.addToHead(9);
l2.addToHead(3);
l2.addToHead(4);

l1.printAll();

cout << "-------------------" << endl;
l2.printAll();

l1.append(l2);

cout << "=============" << endl;

output: 2 1 line of slashes 4 3 9 line of equals press any key to continue...

When I did not account for the other conditions like if l1 is empty or l2 is empty, the code worked fine. It appended l2 to l1, and emptied l2.

But now that I'm trying to account for both lists being empty or one list being empty, I'm getting a debug assertion. It looks like the destructor tries to run, then runs into a problem right before "delete head"

UPDATE--

the isEmpty fn:

    int isEmpty() {
    return head == 0;
}
Jack Faber
  • 37
  • 6
  • Are you using the built-in copy constructor? – owacoder Mar 11 '16 at 22:23
  • 1
    But where is `isEmpty()` defined? That seems to be the most likely point of failure here. –  Mar 11 '16 at 22:28
  • Yeah. Is it l1.append(l2); that is causing the issue? – Jack Faber Mar 11 '16 at 22:29
  • Based on your update, I'm guessing your list is actually corrupted somehow. Check where you define `head->next` outside of appending. It might also be helpful if you either used a debugger or (possibly easier) a debug function to "walk" the list, printing the addresses of `head` and `next` until it reaches `NULL`. That should tell you what is wrong. –  Mar 11 '16 at 22:33
  • I commented out my destructor and the program works fine. When I include it, the debug assertion fails. – Jack Faber Mar 11 '16 at 22:37
  • @JackFaber Well if you comment out the destructor it will never be destructed (during normal run), so that likely means you're just side-stepping whatever structural issue is present. But I'm not sure why `printAll()` would work and the destructor wouldn't. You must be using a different criteria for something and that's probably going to explain what's wrong. –  Mar 11 '16 at 22:40
  • 1
    @JackFaber I'm going out on a limb here and recommending you read [What is The Rule of Three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) – user4581301 Mar 11 '16 at 22:49
  • And also read [How to debug small programs](http://ericlippert.com/2014/03/05/how-to-debug-small-programs/) and [How to create a Minimal, Complete, and Verifiable example](http://stackoverflow.com/help/mcve). – user4581301 Mar 11 '16 at 22:52
  • @JackFaber - Almost assuredly the problem lies in the fact that you did not define a custom copy constructor. Thus, when the list is copied (such as when the argument is passed to `append`), the pointers are copied directly as well. When one list gets deleted, the pointed-to data is deleted as well, and the other list *still holds references to the deleted data*. Attempting to delete the pointers twice will fail. – owacoder Mar 11 '16 at 23:16
  • @JackFaber `void append( list L2)` This will certainly cause trouble if you do not have a user-defined copy constructor. The reason is that you're passing `list` by value, and when you pass by value, the compiler will make a copy of `L2`. – PaulMcKenzie Mar 11 '16 at 23:27
  • What should I define in the copy constructor though? list(list &L2) would be the heading right? What do I add to the body? – Jack Faber Mar 11 '16 at 23:28
  • @JackFaber No. The copy constructor means what it says -- you actually make a copy of the passed-in `list`. In other words, after the copy constructor is invoked, you have two copies of the list, separate and apart, from each other. Also, there is nothing wrong with your destructor. It is the copy constructor and assignment operator that's at issue. – PaulMcKenzie Mar 11 '16 at 23:30
  • okay- am I going in the right direction? Make copy constructor, list (const list & lo). Then in the body make a temporary node and use that to traverse the list, and make head point to a new node that is created with the integer from lo.head? – Jack Faber Mar 11 '16 at 23:42
  • @JackFaber -- You're asking for implementation details, but I don't think you understand in concept what the copy constructor is supposed to do. You are supposed to be able to support this without error: `list list1;...;list list2= list1;` Your code does not support this, as it will cause undefined behavior. You currently cannot copy a list to another list and have two distinct (but equivalent) list objects without memory corruption issues. You need to fix this, and that is done via writing a copy constructor. If this is a homework exercise, you should have been told this. – PaulMcKenzie Mar 12 '16 at 05:55
  • @JackFaber To continue -- it isn't the destructor or the `isEmpty` function that is the problem. Your issue is simple -- write a copy constructor and assignment operator -- you were given the link to the "rule of 3" by a previous commenter. Without those functions you're missing, your code will not work properly if you are going to copy list objects (like you're doing now). It's that cut and dry. Here is what your code needs to be able to do, without error: http://coliru.stacked-crooked.com/a/6bb5fd9bb10e9941 Ignore the compiler errors and take a look at that link. – PaulMcKenzie Mar 12 '16 at 06:00
  • okay, I got the copy constructor to work. New issue- I need the function to empty out L2. But because it is a copy, it's emptying the copy of L2, leaving the original L2 still populated. – Jack Faber Mar 12 '16 at 12:28
  • The only place I used the Copy constructor was in the append function, so after copying the list, I set the copy constructor to set the original list to Null for me. It sounds shady, but it got the job done. – Jack Faber Mar 12 '16 at 12:54

2 Answers2

0

My bet is that isEmpty is checking if head is NULL or not, which is always evaluating to true because you never set it otherwise.

Try this instead:

node* p = NULL;

while (head != NULL)
{
    p = head->next;
    delete head;
    head = p;
}
Colin Basnett
  • 4,052
  • 2
  • 30
  • 49
0

I think the return type of isEmpty() should be boolean

bool isEmpty() {
    return head == 0;
 }
Duy Kha Đinh
  • 58
  • 1
  • 6