Invoking of destructors in C++

Question

The following piece of code has a user defined data type called String.Objects of this class store a char pointer str(short hand for string) and length.

#include<iostream>
#include<cstring>
using namespace std;
class String
{
    char* str;
    int length;
public:
    String(){}                     //default constructor 
    String(const char* s)
    {
        length=strlen(s);
        str=new char[length+1];   
        strcpy(str,s);
    }
    void add(String a,String b)   //function to concatenate strings
    {
        length=a.length+b.length;
        str=new char[length+1];
        strcpy(str,a.str);
        strcat(str,b.str);
    }
    void display()
    {
        cout<<str<<endl;
    }
    ~String()                                //destructor 
    {
        delete str;
        cout<<"Destructor invoked!";
    }
};
int main()
{
    String s1;
    String s2("Well done!");
    String s3("Boy");
    s1.add(s2,s3);
    s1.display();
    s2.display();
    s3.display();
}

Output:

Destructor invoked!Destructor invoked!Well done!boy
X!!;
<!!;
Destructor invoked!Destructor invoked!Destructor invoked!

It appears as though the destructor is invoked even before the display functions get invoked.Why is this?

If the destructor function were not defined, I get the following output(as expected):

Well done!boy
Well done!
boy

Why is this unexpected output upon defining a destructor?

The destructor is called because the arguments of `add` are destroyed. — MikeCAT, Jun 26 '16 at 08:21
And your class is not Rule-of-3 compliant. You need to add at least copy constructor and copy assignment operator. — Revolver_Ocelot, Jun 26 '16 at 08:22
You have both memory leaks *and* double deletes. Look at `add`, and [what is the rule of three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). — juanchopanza, Jun 26 '16 at 08:23
`delete str` should be `delete[] str;` - Also you have a memory leak in `add` — Ed Heal, Jun 26 '16 at 08:23
You also need to initialize members in the default constructor. — interjay, Jun 26 '16 at 08:32
@user7852: Apparently you noted a problem with my answer and removed the selection of it as solution. What was the problem? — Cheers and hth. - Alf, Feb 20 '17 at 16:57

score 1 · Answer 1 · answered Jun 26 '16 at 08:32

1

You need copy constructor for this: void add(String a,String b); It's better to pass strings by reference:

void add(const String &a, const String &b);

Also in destructor you need to free the memory with delete[].

delete[] str;

Now it will work.

answered Jun 26 '16 at 08:32

Lassie

853
8
18

Re "Now it will work.", no, the code still has UB after just adding a copy constructor. – Cheers and hth. - Alf Jun 26 '16 at 08:47

Cheers and hth. - Alf · Accepted Answer · 2016-06-26T09:15:51.547

This default constructor

String(){}

doesn't initialize anything, because the two data members are of fundamental types and a fundamental type doesn't provide automatic initialization.

Any use of the resulting instance will therefore use indeterminate values (of the data members), and will therefore have Undefined Behavior.

Fix: initialize the data members.

You also need to take charge of copying and moving, e.g. to avoid double deletes – which is also Undefined Behavior.

The example code copies String instances in the call

s1.add(s2,s3);

… because add takes the arguments by value.

Fix: define at least a copy constructor.

In the destructor the deallocation

delete str;

… does not match the allocation of str, which used new[].

It can work in practice, but it's Undefined Behavior and therefore can cause all sorts of issues.

Fix: use delete[] for something created with new[].

After fixing the above the code will still leak memory. A simple fix could be to define a concatenating constructor, let add use that to create a new String instance, and then swap the states of that instance and the current instance. This approach has the advantage of automatically (well, almost automatically) being exception safe.

General advice:

Instead of <cstring>, just use <string.h>. One advantage is that like the C header of the same name, it guarantees to place the names in the global namespace. So if you use unqualified names like strcpy it will work portably, not just by happenchance for the compiler at hand.
Instead of passing potentially large objects by value, as in void add(String a,String b), pass them by reference to const, as in void add(String const& a,String const& b), to avoid needless copying.
Preferentially don't do i/o, like the void display() member function, in a class that isn't concerned with i/o. For example, the display() function won't work in a graphical user interface.

Invoking of destructors in C++

2 Answers2