Linked list destructor C++

Question

I'm learning to implement Stack using linked list. This is the node class:

class StudentInfo {
    public:
        string id, name, course;
        double GPA;
        StudentInfo *next;
};

This is the Stack class:

class StackLinkedList {
    public:
        StudentInfo* top; //pointer to point to the top node
        int size; //variable to keep the size of the stack

    //constructor
    StackLinkedList() {
        this->size = 0;
        this->top = NULL;
    }

    //destructor
    ~StackLinkedList() {
        StudentInfo *current = top;
        while (top) {
            current = current->next;
            delete top;
            top = current;
        }
    }

    //to add item into stack - push on top
    void push(StudentInfo *newStudent) {
        if (!top) {
            top = newStudent;
            return;
        }

        newStudent->next = top;
        top = newStudent;
        size++;
    }

void main() {
    StudentInfo s1("phi", "123", "computer science", 4.0);
    StudentInfo s2("abc", "123", "software engineer", 4.0);
    StudentInfo s3("zxc", "123", "business management", 4.0);

    StackLinkedList list;
    StudentInfo *ptr;
    ptr = &s1;
    list.push(ptr);
    ptr = &s2;
    list.push(ptr);
    ptr = &s3;
    list.push(ptr);

};

When I try to run unit test on push() and printAll(), everything is okay. However, after destructor() has been called, and error is showed up Debug Assertion Failed … is_block_type_valid(header-> _block_use). And the debugger triggered a breakpoint at delete top;

//destructor
~StackLinkedList() {
    StudentInfo *current = top;
    while (top) {
        current = current->next;
        delete top; //here
        top = current;
    }
}

If I put top = NULL; before delete top;, the error is gone. So, I have a little bit of confusion about the top = NULL; statement. Edit: Constructor for NodeType

 StudentInfo(string id, string name, string course, double gpa) {
        this->id = id; this->name = name; this->course = course; this->GPA = gpa; this->next = NULL;
}

Recommended read: https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three — user0042, Oct 15 '17 at 18:53
First thing first, try running valgrind and check if there are any memory leaks. — Mayank Jain, Oct 15 '17 at 18:54
1) Don't use raw `new`/`delete`. Use smart pointers (or just plain containers of objects). 2) Don't use `NULL`. Use `nullptr`. — Jesper Juhl, Oct 15 '17 at 18:55
@JesperJuhl is it valid if I try to return nullptr for pointer return type? — Phi Truong, Oct 15 '17 at 19:04
@Phi Truong you *can* return `nullptr`, sure. Whether or not that's what you *want* and what your callers *expect* is a different question. — Jesper Juhl, Oct 15 '17 at 19:07

WhiZTiM · Accepted Answer · 2017-10-15T19:13:15.110

You invoked Undefined Behavior by attempting to delete objects of automatic storage duration.

int main() {
    StudentInfo s1("phi", "123", "computer science", 4.0);
    StudentInfo s2("abc", "123", "software engineer", 4.0);
    StudentInfo s3("zxc", "123", "business management", 4.0);

    StackLinkedList list;
    StudentInfo *ptr;
    ptr = &s1;
    list.push(ptr);
    ptr = &s2;
    list.push(ptr);
    ptr = &s3;
    list.push(ptr);

};

As you can see, s1, s2, s3 are objects of automatic storage duration (aka, the compiler automatically invokes their destructors at the end of their lifetime).

Yet you pass their addresses to list, whose destructor deletes all pointers within its linked-list-detail, upon destruction.... Never call delete on a pointer to an object that wasn't created using new.

Some additional notes:

void main() is illegal in C++. Are you using an older compiler? ..
Every object should manage its resource. A std::forward_list for example manages the allocation of its nodes internally using allocators. I suggest you redesign StackLinkedList to manage its nodes internally, so that clients will never bother about lifetimes.
You should read up on the Rule of Three, and The Rule of Five
There are some other bugs in your code, I've not touched.

One more question, I don't really understand how `top = NULL` can solved this problem? — Phi Truong, Oct 15 '17 at 19:09
after your `top = NULL`, the following `delete top` become a "delete NULL`, which is basically a no op (does not call any dtor, does not free any memory) — Gian Paolo, Oct 15 '17 at 19:12
@PhiTruong, How you can perform unit tests internally would be beyond the scope of answers here. However, you would of cause have lots of test cases for `StackLinkedList` as a *Unit*. ...And as to your second comment, Gian Paolo has explained... — WhiZTiM, Oct 15 '17 at 19:16

Vlad from Moscow · Answer 2 · 2017-10-15T19:18:54.147

For starters you sis not initialize the data member next of objects of the type StudentInfo.

So all the code that relies on that the last node in the list is equal to nullptr will invoke undefined behavior.

Also you may not use the operator delete for objects that were not created with operator new.

So instead of the statements

StudentInfo s1("phi", "123", "computer science", 4.0);
StudentInfo s2("abc", "123", "software engineer", 4.0);
StudentInfo s3("zxc", "123", "business management", 4.0);

you should at least write (I assume that StudentInfo is an aggregate. If the class has a constructor then declare it like

StudentInfo( const string &id, 
             const string &name, 
             const string &course, 
             double gpa, 
             StudentInfo *next = nullptr ) 
{
        this->id = id; this->name = name; this->course = course; this->GPA = gpa; this->next = next;
}

) StudentInfo *s1 = new StudentInfo {"phi", "123", "computer science", 4.0, nullptr}; StudentInfo *s2 = new StudentInfo {"abc", "123", "software engineer", 4.0, nullptr }; StudentInfo *s3 = new StudentInfo {"zxc", "123", "business management", 4.0, nullptr };

and then

list.push(s1);
list.push(s2);
list.push(s3);

@PhiTruong What parameter are you saying? Its class definition is an aggregate. — Vlad from Moscow, Oct 15 '17 at 19:13
@PhiTruong Either remove the constructor because it is redundant due to the fact that all data members are public or make the fifth parameter with the default argument that corresponds to the data member next. — Vlad from Moscow, Oct 15 '17 at 19:15
Thank, I didn't know, I can start an object in C++ without an constructor. — Phi Truong, Oct 15 '17 at 19:17

Linked list destructor C++

2 Answers2