Issue with return value optimisation

Question

i was reading a bit about RVO in c++, and found a weird observation. I ran the below code..

class myClass {
  private:
    int *ptr;
    static int id;

  public:
    myClass() {
      id++;
      ptr = new int[10];
      printf("Created %p id %d this %p\n", ptr, id, this);
    }

    ~myClass() {
      delete[] ptr;
      ptr = NULL;
      printf("Deleted ptr id %d this %p\n", this->id, this);
      id--;
    }
};

int myClass::id = 0;

myClass testFunc(myClass var) {
  myClass temp;
  return temp;
}

int main() {
  myClass var1, var2;

  testFunc(var1);

  return 0;
}

i got the o/p as

Created 0x9b14008 id 1 this 0xbfe3e910
Created 0x9b14038 id 2 this 0xbfe3e914
Created 0x9b14068 id 3 this 0xbfe3e91c
Deleted ptr id 3 this 0xbfe3e91c
Deleted ptr id 2 this 0xbfe3e918
Deleted ptr id 1 this 0xbfe3e914
Deleted ptr id 0 this 0xbfe3e910

the temporary copy variable in the call to testFunc actually causes some issue. it deletes the ptr member of var1, which can be seen by the call to the destructor in the pointer 0xbfe3e918. under valgring this code show no mem leaks but invalid delete[].

i was bit confused how the extra destructor is being called and why no corresponding constructor call for the same ??

Answer 1

This really doesn't have anything to do with return value optimization. RVO would only be noticeable if you were using the result of testFunc.

The issue you are seeing is because your class is just using the defaulted copy constructor, so when var is passed to testFunc, the ptr member gets copied just as a regular pointer, without creating a new copy of the object it is pointing to.

Because of that, you end up with two myClass objects which are pointing to the same underlying int array, and when the destructors of these two objects are called, they try to delete the same int array twice.

Answer 2

Passing var1 as the function argument by value creates a copy. This is done with the (implicitly defined) copy constructor, which is why your program doesn't print anything - you only print something in the default constructor.

You now have a big problem: both copies of the object contain a pointer to the same array, and both will try to delete it when destroyed. This double deletion is an error, causing undefined behaviour.

To fix it, either follow the Rule of Three to make the class correctly copyable (or not copyable); or stop all this dangerous mucking around with raw memory and use std::vector or similar to manage the array correctly for you.

Answer 3

When testFunc is called then var is initialized to be a copy of var1 from main. This is done using the default copy constructor, hence the "missing" constructor call. It was just another constructor was called.

So the problem is that now that var is a copy of var1 that means that var.ptr must have the same value as var1.ptr.

1. The solution is to provide your own copy constructor that deals with this situation. One solution would be make a deep copy:

myClass(const myClass& o) {
    id++;
    ptr = new int[10];
    memcpy(o.ptr, ptr, 10);
    printf("Created copy %p of %p id %d this %p\n", ptr, o.ptr, id, this);
}

2. Another solution would be to use a shared_ptr that can keep track of the number of objects that has copies of a particular pointer:

//int *ptr;           //Replace this
shared_ptr<int> ptr;  //With this

//Initialize ptr:
myClass():ptr(new int[10]) {
    //...
}

// Also eliminate cleenup. It's handled by shared_ptr:
~myClass() {
    printf("Deleted ptr id %d this %p\n", this->id, this);
    id--;
}

The problem with the shared_ptr approach is that they still share the same pointer. Which means that it does not behave as a copy which is what is usually expected. Could be ok if values pointed to by ptr are never changed.

I would prefer solution 1.