2

Consider the following code. This code is taken from the book Object Oriented Programming With C++!-Chapter 12.Templates.

#include<stdio.h>
#include<stdlib.h>
#include<iostream>
using namespace std;
class vc
{
    int size2;
    int *v;
public :

    vc(int size1);
    vc(int *a);
    ~vc()
    {
        printf("\n calling destructor");
    }
    int operator *(vc );
};
vc::vc(int size1)
{
    v = new int[size2=size1];
    for(int i=0;i<this->size2;i++)
        v[i]=0;
}
vc::vc(int a[])
{

    for(int i=0;i<this->size2;i++)
    {
        v[i]=a[i];

    }
}
int vc::operator *(vc v1)
{
    int total=0;
    for(int i=0;i<size2;i++)
        total+=(v[i]*v1.v[i]);
    return total;
}

int main()
{
    int a[3]={1,2,3};
    int b[3]= {5,6,7};
    vc v1(3),v2(3);
     v1=a;
     v2=b;
    int total = v1*v2;
    cout << total;
    return 0;
}

First of all this code is not working properly. It should show 38 as output. When I started debugging this code, I found 3 is assigned to size2 after this line vc v1(3),v2(3);. But while executing the next line, control is passed to the second constructor and size2 shows a garbage value. Moreover,destructor is called after line v1=a and same happens after the next line.

Final output:

calling destructor
calling destructor
calling destructor0

Why does destructor get called 3 times? Is this code wrong?

Heisenberg
  • 1,500
  • 3
  • 18
  • 35
  • 8
    Code from Chapter 12.Templates. Doesn't have RAII. Conclusion: Bad book. – R. Martinho Fernandes Aug 09 '13 at 15:17
  • Well, it does have a section on "Executing Turbo C++", so. :P – cHao Aug 09 '13 at 15:44
  • 1
    @cHao: Being outdated is the least of its problems; teaching incorrect memory management like this is unforgivable. RAII was well understood even in the days of Turbo C++, the "new ANSI standard" and the "Standard Template Library". – Mike Seymour Aug 09 '13 at 16:23
  • 1
    You should throw the book away and and try one of [these](http://stackoverflow.com/questions/388242), if you want to learn to write C++ correctly. – Mike Seymour Aug 09 '13 at 16:24

5 Answers5

8

When you call v1*v2, you pass v2 to the method

int vc::operator *(vc v1)

which creates a local copy of v2. Therefore, you have an additional vc instance.

The answer for your first doubt,

But while executing the next line, control is passed to the second constructor and size2 shows a garbage value and it is not executed for three times.

is because 

v1 = a;

creates a temporary vc by calling vc::vc(int a[]) and assign it to v1. However, this constructor did not initialize size2. So you get a garbage value.

A cleaner approach would be to pass an array as well as its size:

vc::vc(int a[], int size1) {                                                                                       
    v = new int[size2=size1];                                                                                      
    for(int i=0; i<size2; i++)                                                                                     
        v[i] = a[i];                                                                                                 
}

int main()
{
    int a[3]={1,2,3};
    int b[3]= {5,6,7};
    vc v1(a, 3), v2(b, 3); 
    int total = v1*v2;
    cout << total;
    return 0;
}

Then total will be 38.

Yang
  • 7,712
  • 9
  • 48
  • 65
5
v1=a;

That line actually isn't only an assignment. It's the construction of a temporary object, and the assignment of that to your already created object. The only way to assign int[] to your class which the compiler can see, is to use the vc(int a[]) constructor to create a temporary object of vc - but that constructor doesn't initialize size2, that's the problem you're seeing ("But while executing the next line, control is passed to the second constructor and size2 shows a garbage value.").

After this temporary is created, the implicit assignment operator (automatically created for you by the compiler, since you didn't specify one) will copy the members of this object to your already created object.

This is called implicit conversion (see e.g. here: https://publib.boulder.ibm.com/infocenter/lnxpcomp/v8v101/index.jsp?topic=%2Fcom.ibm.xlcpp8l.doc%2Flanguage%2Fref%2Fcplr384.htm). If you want to prevent that from happening, use the explicit keyword on the vc(int a[]) constructor.

Afterthought: My initial answer didn't really answer the main question stated at the end (why the destructor is called three times). I guess that's been answered by others here already quite nicely: One vc object is created locally inside your operator* because of the parameter-passing by value you do there.

Now you could ask why the destructor isn't actually called 5 times (2 times as well for the implicitly created objects). I guess it's because the Compiler optimizes the actual creation of a new object away somehow (could be Return Value Optimization, someone please correct me if I'm wrong!).

codeling
  • 11,056
  • 4
  • 42
  • 71
3

v1*v2 creates a third temporary object, thus its destructor is also called as well as the two objects that you create.

bizzehdee
  • 20,289
  • 11
  • 46
  • 76
2

You should pass by reference rather than value thats the reason for the extra destructor.

   int vc::operator *(vc &v1)

And, from the spot on answer of nyarlathotep. Apart from adding an explicit keyword, which would prevent the bug.

You are just missing this

void vc::operator=(int* a)
{
    for(int i=0;i<this->size2;i++)
{
    v[i]=a[i];

}
}
innosam
  • 427
  • 1
  • 5
  • 18
1

As others have said, the destructor is called three times due to the fact that when operator*(vc v1) is called, a temporary local copy is constructed because it's accepting argument v1 by value.

The answer that nyarlathotep gave is correct. The suggestion by innosam is almost ideal.

I wanted to add a few other more concrete points.

Two things to note:

  • When passing instances of objects that you merely want to reference inside of the called function (as opposed to copy or change), as is the case with this operator* method, always pass by reference-to-const: operator*(const vc& v1)
  • v1 = a does invoke the constructor vc(int a[]) only due to implicit conversion of integer array a to an instantiation of vc as nyarlathotep mentioned. You can avoid this by adding the keyword explicit to this constructor, which instructs the compiler not to allow such conversions.

Keeping the example in main the same, the fewest number of code changes to produce the answer you're looking for (38) are as follows:

  • Change operator* to take an argument of type const vc&.
  • Define a custom assignment operator: vc& operator=(int src[]). Its implementation can be exactly the same as that of the constructor vc(int a[]).

You'll notice that the constructor vc(int a[]) will no longer be called with these two changes.

Furthermore, although I know that this is just an example from a book (and to echo nyarlathotep, hopefully it is an example of what not to do), there are additional problems with this class and I'd like to point out a couple of them:

  • Memory cleanup (i.e. delete[]) should be added to the destructor.
  • Since this class dynamically allocates memory to a member variable, the default copy constructor and default assignment operator are not sufficient because they only perform shallow copies of member variables, not deep copies. This behavior is not invoked given the code you've provided (with the two changes I mentioned above), but keep this in mind for the future.
Community
  • 1
  • 1
nklauza
  • 1,501
  • 1
  • 12
  • 12