Is the destructor deleting the correct instance

Question

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");
}

`new[]` *must* be matched with `delete[]`. As any decent [book](https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list), tutorial or class should have taught you. Otherwise you will have *undefined behavior*. — Some programmer dude, May 15 '23 at 02:37
You're using the wrong `delete`, so this program has undefined behavior. You need _array-delete_. As an aside, use array syntax (for code readability) instead of pointer arithmetic. _i.e._ `pArray[0] = 0; pArray[1] = 0; pArray[2] = 0;` or consider using a loop or something like `std::fill`. Also, read about [The Rule of Three](https://stackoverflow.com/q/4172722/1553090) which is important here. Otherwise you can end up with double-deletion. — paddy, May 15 '23 at 02:39
@Someprogrammerdude So, you are saying that you can't allocate an object on the stack, which is what my code is doing. — Bob Bryan, May 15 '23 at 02:41
That is not at all what they said. In fact, they didn't even mention the stack. — paddy, May 15 '23 at 02:42
Assigning to a variable does *not* cause the variable to point to the new instance. Instead, it copies the new instance over the existing instance; this is because C++ has **value semantics**. See https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three/4172724#4172724 — ecatmur, May 15 '23 at 02:44
No, what I was saying in my comment is that `delete pArray;` should have been `delete[] pArray;` — Some programmer dude, May 15 '23 at 02:46
And your new updated code is no longer "buggy". As currently show it will work just fine as you removed the source of all your problems. That makes my answer kind of meaningless. Perhaps you should ask another question, why multiple objects are create and destructed, when you think you only create one object? — Some programmer dude, May 15 '23 at 02:48
And on another note, don't overuse `inline`. When you define (implement) member function inside the class they already *are* inline. The `inline` keyword does nothing in that cotext. — Some programmer dude, May 15 '23 at 02:50
@Someprogrammerdude Thank you. My C++ is a little rusty. I have accepted your answer. — Bob Bryan, May 15 '23 at 02:54
The edited version of the question is not about the Rule of Three. It's a potential memory leak if `ShowBug` allocates memory. Since C++ is not generally garbage-collected, the second assignment will leak any memory made by the first assignment. (The compiler or optimizer may realize that the first objects is not used and fix this.) — Andrew Lazarus, May 30 '23 at 04:30

score 5 · Accepted Answer · answered May 15 '23 at 02:42

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.

`ShowBug sb = ShowBug("First Instance");` creates only one object; `sb` is initialized directly by the constructor. See http://eel.is/c++draft/dcl.init#general-16.6.1 — ecatmur, May 15 '23 at 15:49

Is the destructor deleting the correct instance

1 Answers1