3

I have a class whose members are somewhat as follows

CRectangle CRectangle::operator+ (CRectangle param) {
    CRectangle temp;
    temp.width = new int;
    temp.height = new int;
    *temp.width = *width + *param.width;
    *temp.height = *height + *param.height;
    return (temp);
}

CRectangle& CRectangle::operator= (CRectangle param) {
    *width =  *param.width;
    *height = *param.height;
    return *this;
}

CRectangle::~CRectangle () {
    n--;
    delete width;
    delete height;
}

Now if i do

CRectangle a(3, 4), b(5, 6), c;
cout << b.area() << endl; // gives 30
c = a + b;
cout << b.area() << endl; // gives random value

then destructor is called on object b too. I mean, it should be called on temp, but why on b? Is this related to how i am passing the parameters?

e-sushi
  • 13,786
  • 10
  • 38
  • 57
Akshat Aggarwal
  • 89
  • 1
  • 14
  • 1
    The lifetime of the members width and height are not the same as the lifetime of CRectange, which can cause all kinds of obscure memory problems. Use 'int' as the type of width and height instead of int* to simplify your code. You dont need the new int and delete width/height anymore. This probably fixes the problem as well. – Jasper Nov 24 '13 at 14:20
  • actually i m just testing the destructor functionality... So, i wrote the code this way – Akshat Aggarwal Nov 24 '13 at 14:23
  • I guess you haven't written a copy constructor. Testing destructor functionality without a valid copy constructor is a recipe for trouble. – john Nov 24 '13 at 14:29
  • m new to oops in c++, can you guide me a little? – Akshat Aggarwal Nov 24 '13 at 14:31
  • Correct way to add an addition operator to the classes like yours is adding an operator overload of signature of `const CRectangle operator+(const CRectangle& lhs, const CRectangle& rhs);` as a global function. And if that function needs to access private members of parameters, declare it `friend`. – Fredrick Gauss Nov 24 '13 at 14:49
  • 1
    google rule of three c++ -- without that fundamental change other changes are bandaids. You need to implement or disable your copy and or move constructors. – Yakk - Adam Nevraumont Nov 24 '13 at 14:53
  • It's also on SO: [Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) – πάντα ῥεῖ Nov 24 '13 at 14:54

2 Answers2

3

Because you're not passing a const-reference to the operator function (and you should be). Also the function should be const too:

CRectangle CRectangle::operator+ (const CRectangle &param) const {...}
dalle
  • 18,057
  • 5
  • 57
  • 81
Eutherpy
  • 4,471
  • 7
  • 40
  • 64
2

Your operator+ receives copy of the argument passed to it.

    CRectangle CRectangle::operator+ (CRectangle param) {
        CRectangle temp;
        temp.width = new int;
        temp.height = new int;
        *temp.width = *width + *param.width;
        *temp.height = *height + *param.height;
        return (temp);
    }

In the code:

CRectangle a(3,4),b(5,6),c;
c=a+b;

More specifically, in the a + b copy of b is passed to a.operator+. At the end of the function this copy is destroyed.

So, to sum up:

  • Your statement c = a + b; can be separated in two parts:

    temp = a + b;
    c = temp;
    
  • The first statement is basically this:

    temp = a.operator+(b);
    

Now, your operator+ is receiving param by value. This means that copy of object b will be created and you will have two identical objects: b and param. When this function returns, param object will be destroyed.

Now, everything would be good except:

  • Even b and param are separated objects, their pointers width and height points to a single memory location. Destructor of param will free this memory and b's pointers will point to deallocated memory locations. In order to prevent this, you need to write copy-constructor:

    CRectangle::CRectangle(const CRectangle& param) {
        width = new int;  // alocate separate memory locations!
        height = new int; 
        *width =  *param.width;
        *height = *param.height;
    }
    

To avoid copy-on-passing, pass by const-reference instead:

CRectangle CRectangle::operator+ (const CRectangle &param)

As you noted, the same thing happens with operator=.

Also, this code:

temp.width = new int;
temp.height = new int;
*temp.width = *width + *param.width;
*temp.height = *height + *param.height;

can be rewritten as (also the types of width and height should be changed to int):

temp.width = width + param.width
temp.height = height + param.height;

There's no need to have pointers as fields in your class, if they serve just like regular values.

Nemanja Boric
  • 21,627
  • 6
  • 67
  • 91
  • but it would receive the copy of temp right. why b is deleted? – Akshat Aggarwal Nov 24 '13 at 14:22
  • @AkshatAggarwal `b` is not deleted Copy of the `b` is deleted - see my edit for more explanation. – Nemanja Boric Nov 24 '13 at 14:24
  • see i have an area function which multiplies width and height. So, if i call it on b before c=a+b, it gives 30, but after it gives a random value. If the copy is deleted, why is that happening? See the edit – Akshat Aggarwal Nov 24 '13 at 14:32
  • ok got it thanks :) i think what is happening is, when i m deleting the copy, it is also deleting the height and width pointer's data. So there are returning random values – Akshat Aggarwal Nov 24 '13 at 14:37
  • You need to write down copy-constructor, so it would create separate memory on copy. Or you may just not use pointers here. – Nemanja Boric Nov 24 '13 at 14:40