C++ Dynamic array destructor error

Question

I have a sparse matrix class with rows and columns. The rows integer is used to initialize a numbers of LinkedList in a dynamic array.

template<class T>
SM<T>::SM(int rows, int columns)
{
    this->rows = rows;
    this->columns = columns;
    this->rowList = new LinkedList<T>[rows];
    cout << "Going to create a sparse matrix of dimensions " << this->rows << "-" << this->columns << endl;
}

I also have this copy constructor to be used later on:

EDIT:

LinkedList copy constructor:

LinkedList(const LinkedList & other) {
    this->size = other.size;
    this->head = NULL;
    this->tail = NULL;
    NodeType<T> * current = other.head;
    while (current != NULL) {
        setValue(current->info, current->index);
        current = current->link;
    }
}

SparseMatrix copy constructor:

template<class T>
SM<T>::SM(const SM<T> & other)
{
    this->rows = other.rows;
    this->columns = other.columns;
    this->rowList = new LinkedList<T>[this->rows];
    for (int i = 0; i < this->rows; i++)
    {
        rowList[i] = other.rowList[i];
    }
}

This is my LinkedList destructor and SparseMatrix destrcutor:

~LinkedList() {
    cout << "Going to delete all " << size << " elements of the list." << endl;
    NodeType<T> * current = head;
    while (current != NULL) {
        current = current->link;
        delete head;
        head = current;
    }
}

template<class T>
SM<T>::~SM()
{
    cout << "Deleting sm" << endl;
    delete [] rowList;
    rowList = NULL;
}

However, when I'm done with the code. I get a destructor error.

Here is my main() :

SM<int> sm(rows, columns);
SM<int> sm2(rows, columns);
SM<int> sm3 = sm2;

This is the error:

_CrtIsValidHeapPointer

I'm new to C++ and I have really no idea what's wrong with my code. Any help is deeply appreciated.

Have you implemented a valid copy assignment operator for `LinkedList`? — R Sahu, Dec 29 '15 at 05:12
You are calling functions `readElements` and `printMatrix`. If these functions do not contribute to the issue, remove them and rerun your code. Otherwise, post these functions. — PaulMcKenzie, Dec 29 '15 at 05:14
Also, what does `LinkedList`'s destructor do? If it also traverses the link (so deleting one also deletes all the linked nodes), then you should not explicitly do that in `SM`'s destructor. Deleting an object twice yields undefined behaviour. — Peter, Dec 29 '15 at 05:16
duplicate of this question http://stackoverflow.com/questions/10819550/debug-assertion-failed-crtisvalidheappointerpuserdata — someone, Dec 29 '15 at 05:21
Let's see your `LinkedList` user-defined copy constructor and assignment operator. It better have an assignment operator for this line to work properly: `rowList[i] = other.rowList[i];` — PaulMcKenzie, Dec 29 '15 at 05:25
@Kelok Chan you should share a Short, Self Contained, Correct (Compilable) program, which will make really easy to understand what issue you are facing. — Nik, Dec 29 '15 at 05:26
The error happens when destructor is called for sm3, a variable I created using copy constructor. I have also implemented copy constructor to my LinkedList class but of no avail. — Kelok Chan, Dec 29 '15 at 05:33
You could save yourself a lot of headache by using `std::list` instead of reinventing the wheel; and having SM contain `vector>`. Then you would not need to write any special functions of your own, saving much time in both coding and debugging — M.M, Dec 29 '15 at 05:35
@KelokChan So where is your `LinkedList` assignment operator? You don't have one? Then that's an error in the line I posted in my previous comment, `rowList[i] = other.rowList[i];` See:. http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three — PaulMcKenzie, Dec 29 '15 at 05:37
@PaulMcKenzie... In my view culprit is Linkedlist destructor. current is pointing to head, then updating current and deleting head. `_CrtIsValidHeapPointer` error itself stating that allocation and deallocation memory having issue. Correct me if my doubt is wrong. — someone, Dec 29 '15 at 05:41
@someone So that begs the question of why `LinkedList` was not fully tested before being used in a larger program (if the destructor is bad). Second, even if the destructor were running perfectly, the issue I see is that there is a double deletion error when the destructor is issued due to a lack of an assignment operator for `LinkedList` (at least one has not been posted as of now). — PaulMcKenzie, Dec 29 '15 at 05:44
Assignment operator is yet to be implemented. As of now, I have no idea what an assignment operator is. This is part of my school assignment and I am still learning C++. I will continue my research on the rules of three now. Sorry for the confusion — Kelok Chan, Dec 29 '15 at 05:47
@KelokChan Well, you have to implement the assignment operator since the copy constructor relies on it. The assignment operator takes an existing object and assigns to it another existing object. Look at your loop in the copy constructor -- what happens at that line `rowList[i] = other.rowList[i];`? You are copying pointer values from one object to another. So what happens when `*this->LinkedList*` and `other.LinkedList` go out of scope? The destructors are called. So what pointers will the`LinkedList` destructor be deleting? Ooops. the same pointers! — PaulMcKenzie, Dec 29 '15 at 05:52
@KelokChan For debugging, `cout` the pointer values in the `LinkedList` destructor. You should see that for two different `LinkedList` objects, the same pointers will be deleted twice, causing undefined behavior (and most likely, a crash) to occur. But you haven't learned about the assignment operator? And you're writing templated code? Your course missed several chapters in a basic C++ book. — PaulMcKenzie, Dec 29 '15 at 05:56
Well the module name is Data Structure. Lecturer mentioned that he won't go into too much details on C++ so.. — Kelok Chan, Dec 29 '15 at 06:08
Well, with C++, is isn't just the theory of data structures that you need to know. It isn't as if you can just take what's in a general data structures book, and verbatim implement what you see when it comes to C++. There are a whole host of issues to consider, apart from data structure theory that you need to know. Other languages, like Java, are more suited for the quick and dirty implementation without the dynamic memory allocation issues. See my answer as to how to implement the assignment operator. BTW, you need to do this for `SM` also (assignment operator). — PaulMcKenzie, Dec 29 '15 at 06:11

score 0 · Accepted Answer · edited May 23 '17 at 10:28

One issue is that your LinkedList class lacks an assignment operator., namely

LinkedList<T>& operator=(const LinkedList<T>& other);

The reason why you need this functions is that without it, assignments like this (from your constructor):

rowList[i] = other.rowList[i];

will create shallow copies (copy the internal pointer values), which is not what we want. Since rowList[i] and other.rowList[i] are separate objects, they must have separate pointers internally. If not, the destructor for rowList[i] and other.rowList[i] will use the same pointer values when issuing calls to delete, thus causing undefined behavior.

Let's assume that your LinkedList copy constructor and destructor are working correctly. The easiest way to implement the assignment operator is using the copy / swap idom:

#include <algorithm>
//...
LinkedList<T>& operator=(const LinekedList<T>& other)
{
   LinkedList<T> temp(other); // uses copy constructor
   std::swap(temp.size, size);
   std::swap(temp.head, head);
   std::swap(temp.tail, tail);
   return *this;
}

Note that all we did was create a temporary, and swap out all of the members from the temporary with this, and returned *this.

Note that you will need to implement the assignment operator for the SM class, and to keep it simple, using the same technique (copy / swap).

C++ Dynamic array destructor error

1 Answers1