Search code examples
c++visual-c++destructor

Is the destructor deleting the correct instance


I am using Visual C++ in the Community edition of Visual Studio 2022 with the latest updates. In the C++ code below, main creates an instance of ShowBug and assigns it to the variable sb. The next line creates a 2nd instance of ShowBug and also assigns it to sb.

Before the 2nd line completes, it calls the destructor. But, it does not destruct the first instance, which is what I thought it would do, but instead destructs the newly created 2nd instance. Try it yourself if you find it hard to believe.

So, am I missing something here (i.e. is using the same variable to assign a new instance a bad programming practice?), or is the compiler somehow doing the correct thing? Or, is this a compiler bug?

// ShowBug.h:

using namespace std;
#include <iostream>
#include <string>

class ShowBug
{
   // This class is used to show that the destructor seems to be called for the wrong instance.
   //
   string S;
   int *pArray;
public:
   inline ShowBug(const string &s) : S(s)
   {
   }

   inline ~ShowBug()
   {
      std::cout << "Deleting " + S;
   }
};

int main()
{
   ShowBug sb = ShowBug("First Instance");
   sb = ShowBug("Second Instance");
}

Solution

  • When you do

    sb = ShowBug("Second Instance");
    

    the expression ShowBug("Second Instance") creates a temporary object of type ShowBug. This temporary object will be immediately destructed once the full expression (the assignment) is finished.

    This of course leads to problems, as its destruction also free the memory allocated. The pointer will be copied by the assignment, but only the pointer itself and not the memory it points to.

    So after the assignment the pointer sb.pArray will no longer be valid. Besides the use of the wrong delete operator, attempting to delete the invalid pointer again leads to undefined behavior.

    The solution is to follow one of the rules of three, five or zero. I recommend the rule of zero, by using a std::vector<int> or in your case std::array<int, 3> instead of the dynamic allocation you do now. Then you can remove the destructor.

    If you must use pointers and dynamic allocations, then you need to follow at least the rule of three, and implement a copy-constructor as well as a copy-assignment operator.


    I also overlooked that the initialization

    ShowBug sb = ShowBug("First Instance");
    

    have the exact same problem: You create a temporary object, which is used to copy-construct the sb object. The temporary object is then destructed, taking the memory and original pointer with it.

    The solution is still the same though: The rule of zero, three or five.