Unable to chain overloaded assignment operator despite returning a reference

Question

edit: I have now fixed the issue, I was stupidly deleting the whole bag in my overloaded operator and then trying to insert value into it. I wrote a function to clear the bag without deleting it, and now it works perfectly. Thank you everyone for your help. The original is as follows, in case anyone else ever finds his or herself making the same mistake:

I've seen a lot of other beginners having problems chaining an overloaded = operator, and their problem is almost always forgetting to return a reference in their overloaded operator. However, I believe I am doing so but I am still unable to correctly chain my overloaded + operator.

For example, if I create two MagicBags, mb1 and mb2, then I set

mb1 = mb2;

this compiles and runs correctly. When I then create a third MagicBag, mb3,

mb3 = mb2 = mb1;

won't compile, and I get a null pointer in my overloaded operator.

Here is my operator:

MagicBag& operator=(const MagicBag& other) {
    if (this == &other) {
        return *this;
    }
    this->~MagicBag();//clear calling bag
    node<T> * a;//construct a temp node pointer to use as an iterator to loop over original bag and copy its contents
    int tempSize = other.size - 1;
    while (tempSize >= 0) {
        a = other.first;
        for (int i = 0; i < tempSize; i++) {
            a = a->next;//get my null pointer here
        }
        tempSize--;
        insert(a->value);
    }
    return *this;
}

the destructor is

~MagicBag() {
    if (first == NULL)
        return;
    if (first->next == NULL) {
        delete(first);
        first = NULL;
        return;
    }
    node<T> * a = first->next;
    while (a != NULL) {
        delete(first);
        first = a;
        a = a->next;
    }
    delete(first);
    first = NULL;
}

and the insert function is your standard 4 line insert, I doubt that's the issue. What am I doing wrong? I believe my destructor is not the problem, however I am a beginner so it very well may be, and I also believe I am correctly returning a reference in my overloaded operator. In this commonly cited post, I seem to be following the standard convention for overloading so I'm confused why this works correctly once, but not when chained. I also saw in the linked article from that post, that it's unnecessary to check if the two arguments are the same, however removing that bit doesn't solve the problem. What have I done wrong here?

Answer 1

First off, the code should compile! If it gets to a state where it actually runs and you "get a null pointer" it is a different kind of error but compilation is done.

The code itself is riddled with problems. Here are those I spotted (the last one is causing the issue you noticed):

1. Calling the destructor on thisis always wrong. There are a few cases where it is necessary to explicitly call the destructor of an object but all of these involve explicitly constructing an object in a dedicated piece of memory, typically due to use an overloaded operator new(). If you really need to use the functionality of the destructor elsewhere, move it into a dedicated function, e.g., a function called clear(), and call that from the destructor as well as everywhere else it is needed.

2. The assignment operator has a performance problem: iterating through something like a list is slow. Iterating over [parts of] a list for each element is very slow. It is actually easier to iterate over the nodes in the right hand side just once an insert() each node's value. That is, the core of assignment could simply be

   for (node<T>* current = other.first; current; current = current->next) {
       insert(current->value);
   }
   

   Since this is the same code as is needed for the copy constructor, I'd actually implement the assignment using the copy&swap idiom:

   MagicBag& MagicBag::operator= (MagicBag const& other) {
       MagicBag(other).swap(*this);
       return *this;
   }
   

   where swap() just swaps the contents of all members.

3. It isn't strictly an error but your destructor is way too complex (as a result I can't be sure that it is correct). I don't know all of your class but it seems it could look like this:

   ~MagicBag() {
       for (node<T>* current = first; current; ) {
           node<T>* tmp = current->next;
           delete current;
           current = tmp;
       }
   }
   

4. From the looks of your code, you get a null pointer dereference because neither your destructor nor your assignment operator adjusts this->size: the code isn't shown but I guess your insert() function merely increases this->size. As a result, after calling the object's destructor you may have a non-zero this->size in the assignment which gets incremented once for each new object. Since the loop in the assignment moves over other.size objects it will eventually dereference a null pointer if there are actually fewer nodes than other.size implies. That is exactly the case when the right hand side was non-empty when it was assigned to. Assuming you create a clear() function, this should set this->size to zero (as well as setting this->first to a null pointer after deleteing the nodes).