I want to implement 2 array addition, but when a destructor to the class SList

void operator (SList list2) {
        int totalLen = this->len   list2.len;
        char** temp = new char* [totalLen];
        for(int i = 0; i < len; i  ) {
            temp[i] = this->list[i];
        }
        for(int i = len, j = 0; i < totalLen; i  , j  ) {
            temp[i] = list2.get(j);
        }
        delete[] this->list;
        this->list = temp;
        this->len = totalLen;
        cout << len << endl << endl;
    }

Here are the get method that just return the dynamic array of char:

char* get(int i) {
        if (i >= len) {
            return "";
        } else {
            return list[i];
        }
    }

here are my class SList private variables:

private:
    char** list;
    int len;
    char* generateString(){
        char* str;
        int n = rand() % 20   1;
        str = new char[n   1];
        for(int i = 0; i < n; i  ) {
            str[i] = 'a'   rand()&;
        }
        str[n] = '\0';
        return str;
    };

~SList() {
        delete[] list;
    }

It always shows malloc error on the destructor.

malloc: *** error for object 0x105007410: pointer being freed was not allocated
malloc: *** set a breakpoint in malloc_error_break to debug

Please help! I have carefully checked my delete method on the dynamic allocated array, but it always shows this error.

I have tried to check other delete from the other function, but none of them make the same malloc error. I have tried to commented the destructor method and everything work fine. But i really need to have destructor method here. I hope someone with more expertise on c can help me fix this malloc error and gave an explanation on which part i made a mistake.

CodePudding user response：

Regardless what are other details of implementation, the destructor is not correct while you're using a data structure known as a "ragged array", i.e. list is a pointer to an array of pointers. delete[] would free the pointer array, but not char arrays pointed by its elements. You have to do something like this:

~SList() {
    if(!list) return;   // if we cannot guarantee that list isn't 
                        // nullptr we have to check it, 
                        // or result of list[i] would be undefined. 
    for(int i = 0; i < len; i  ) 
        delete[] list[i];
    delete[] list;
}

and you have to make sure that any of those pointers is either initialized by new expression or is equal nullptr.. It doesn't happen on its own. You have to make sure during construction and all operations. You didn't show any. Look for faults there.

The method get() is a disaster waiting to happen and is ill-formed, i.e. it doesn't follow C rules. The string literal "" returns const char*, always the same one and the statement return ""; is incorrect - some compilers only warn about it though. It cannot be deallocated by delete.

char* get(int i) {
    if (i >= len) {
        return nullptr;  // "" - is not safe
    } else {
        return list[i];
    }
}

Deleting a nullptr is a safe no-op. Deleting something that was't returned by new is a disaster.

The addition operator is taking list2 by value, which means that proper copy operations have to be implemented. You didn't show them either. Default implementation would just copy a pointer and destruction of local copy would deallocate memory used by original via ~SList() above. The operator have to return resulting object and should not modify one pointed by this. You had implemented an operator =. The way you did it, it would work as

 a b;  // a now contains result of concatenation.

It's simply weird to use. Proper operator would be

SList operator (SList& list2);

In general, an object that deals with ownership of some resource, dynamic memory in our case, have to implement certain set of special member functions:

 ~SList();
 SList(const SList& other);
 SList(SList&& other); // move copy, source is a temporal.
 SList& operator=(const SList& other);
 SList& operator=(SList&& other); // move assignment

If that is done right, you can safely do the c = a b; assignment.

Note that if you pass argument by reference you have to take in account that arguments of assigning operators aren't referencing the object pointed by this and act accordingly if they are. Otherwise you would destroy it and loose original data. On other hand copying argument is excessive and user-unfriendly due to increased cost and memory footprint. Concatenation of n-element and m-element array is expected to have memory footprint of n m elements , not n 2*m.