C++ return by value - what happens with the pointer inside?

Question

So I'm having a bit of SegFaults and was wondering if someone can explain me in higher depth how does this work.

I have a wrapper class around a numeric variable, and I'm trying to build a compute tree of the expression. Here is a sample code:

class DoubleWrap{ 
public:
   static int id;
   DoubleWrap(double value){this->value = value;myid=++id;}
   double value;
   int myid;
   std::vector<DoubleWrap*> parents;
   DoubleWrap operator+(DoubleWrap& x){
      DoubleWrap result(this->value + x.value);
      setParents(*this,result);
      setParents(x,result);
      return result;
   }
   void setParents(DoubleWrap& parent,DoubleWrap& child){
      child.parents.push_back(&parent);
   }
   void printTree(){
      std::cout<< "[" << this->myid << "]-(";
      for (auto& elem: this->parents)
          std::cout<< elem->myid << "," << elem <<",";
      std::cout<<")"<<std::endl;
      for (auto& elem: this->parents)
          elem->printTree();
  }
}

The problem I got is for an expression like x = a+b+c+d; When I try to look for the parents of x, it has 2 parents, but parents[0].parents[0] is nil, e.g. for some reason the pointer to the allocated place does not work or the allocated memory is destroyed?

I'm quite interested if anyone can advise me on a workaround and why this happens.

To give an example on this code:

DoubleWrap dw(12.6);
DoubleWrap dw2(12.5);
DoubleWrap dw3(12.4);
DoubleWrap dw4(12.3);
DoubleWrap a = dw + dw2 + dw3;
DoubleWrap b = a + dw+ dw2+ dw3 + dw4;
b.printTree();

I expect the output:

[10]-(9,0x13a4b50,4,0x7fff5bdb62f0,)
[9]-(8,0x13a4b00,3,0x7fff5bdb62c0,)
[8]-(7,0x13a49f0,2,0x7fff5bdb6290,)
[7]-(6,0x7fff5bdb6320,1,0x7fff5bdb6260,)
[6]-(5,0x13a4db0,3,0x7fff5bdb62c0,)
[5]-(1,0x7fff5bdb6260,2,0x7fff5bdb6290,)
[1]-()
[2]-()
[3]-()
[1]-()
[2]-()
[3]-()
[4]-()

But the result I get (usually different on different runs):

[10]-(9,0x7ffffd5e2f00,4,0x7ffffd5e2de0,)
[9]-(33,0x1c6da10,3,0x7ffffd5e2db0,)
Process finished with exit code 139

My guess is that the return of the operator in fact copies the DoubleWrap variable, thus value to which the pointer in parents is pointing to goes out of scope and the memory is released?

The temporary solution was to return by reference, but the question is why and is there a proper one?

PS: Fixed a previous mistake I got which is a mistake in setParents.

just unrelated, but you made my day with "a bit of SegFaults" :)) — vsoftco, Mar 21 '15 at 23:02
@NeilKirk good point, one should (probably) pass by `const` reference just to make sure the `operator+=` is well behaved (i.e., should do the same thing as it does for PODs such as `int`) — vsoftco, Mar 21 '15 at 23:04
That code would not compile, your vector wants pointers-to-objects and you are pushing back objects into it. — Jonathan Potter, Mar 21 '15 at 23:14
Well it does compiles as it takes the objects as a reference which is special type of a pointer. Also I'm not modifying the arguments, but rather the result, I can make it with const arguments, I just need to read part of them. Any suggestions? — Alex Botev, Mar 21 '15 at 23:18
Okay, okay I admit made a bloody mistake since didn't copy paste all of it. I edit the post - should be push_back(&parent); — Alex Botev, Mar 22 '15 at 00:04

Emil Laine · Accepted Answer · 2015-03-22T02:47:54.913

1

Your problem is that you're adding pointers to temporary objects to the vector.

DoubleWrap a = dw + dw2 + dw3;

The above will first call dw.operator+(dw2). That creates a temporary object, let's name it temp. It then calls temp.operator+(dw3). Now inside operator+ there's this call to setParents where you pass temp as the parent. You then take the address of temp (at least that's what the code should be doing in order to compile) and add it to the vector of child.

Now when the call to operator+ returns, temp gets destroyed, and the vector in a contains a pointer to a non-existing object.

You could solve this by storing the elements in the vector using shared_ptr. Note that if referential cycles are possible you should be careful to break those cycles with weak_ptr (I didn't include an implementation for that in the below code). Here's what the final code could look like:

class DoubleWrap {
public:
   static int id;
   DoubleWrap(double value) : value(value), myid(++id) {}
   double value;
   int myid;
   std::vector<std::shared_ptr<DoubleWrap>> parents;
   DoubleWrap operator+(const DoubleWrap& x) const {
      DoubleWrap result(this->value + x.value);
      setParents(*this, result);
      setParents(x, result);
      return result;
   }
   static void setParents(const DoubleWrap& parent, DoubleWrap& child) {
      child.parents.push_back(std::make_shared<DoubleWrap>(parent)); // makes a copy of parent
   }
   void printTree() const {
      std::cout << "[" << this->myid << "]-(";
      for (const auto& elem: this->parents)
         std::cout << elem->myid << "," << elem.get() << ",";
      std::cout << ")" << std::endl;
      for (const auto& elem: this->parents)
         elem->printTree();
   }
};

int DoubleWrap::id = 0;

edited Mar 22 '15 at 02:47

answered Mar 21 '15 at 23:57

Emil Laine

41,598
9
101
157

1

`std::vector` causes undefined behaviour; standard containers cannot have an incomplete type as the template parameter. [See here](http://stackoverflow.com/questions/5691740/initialising-a-struct-that-contains-a-vector-of-itself) – M.M Mar 22 '15 at 00:56
1

I don't think there's an easy solution to this. It could use a vector of `shared_ptr` but I'm worried that referential loops might occur. (and this also constrains the calling code to not use automatic allocation for any DoubleWraps which defeats the purpose). `reference_wrapper` just wraps a pointer so the same lifetime problem would remain – M.M Mar 22 '15 at 01:05
@MattMcNabb Why does this compile if `DoubleWrap` is an incomplete type? Surely the compiler has all the information here, its not a forward declaration after all. – Galik Mar 22 '15 at 01:09
@Galik see link in my first comment. Or [here](http://stackoverflow.com/questions/8329826/can-standard-container-templates-be-instantiated-with-incomplete-types) is another question on the same topic – M.M Mar 22 '15 at 01:52
Okay, but then how can I preserve the object when it goes out of scope? One way that works is to return reference from the operator+, yet a lot of people advice me not to do that? Is it bad. And btw the vector is of type pointer to DoubleWrap, is that bad? – Alex Botev Mar 22 '15 at 02:14
You could use `shared_ptr`s as suggested by Matt. If reference cycles are possible in what you're doing then you'd also have break those cycles with `weak_ptr`. `unique_ptr` wouldn't work here since a `DoubleWrap` can have multiple childs. If you wanna live dangerously you could use `new` and try to manually get each allocated object deleted properly. If I were you I'd probably redesign the whole thing somehow. – Emil Laine Mar 22 '15 at 02:21
I just tried it with `shared_ptr` and it produces the correct output. If there's no possibility for cycles then this is the solution I'd say. I'll edit it into the answer. – Emil Laine Mar 22 '15 at 02:33
That would be great, since it is a compute tree there will never be a cycle (potentially should make maybe parents and others protected so users can't intentionally introduce such) . Thanks! – Alex Botev Mar 22 '15 at 14:02

score 0 · Answer 2 · answered Mar 21 '15 at 23:49

0

Your code : DoubleWrap operator+(DoubleWrap& x){ DoubleWrap result(this->value + x->value); would be correct if x was a pointer to a DoubleWrap class but you have coded x as a DoubleWrap class object and not a pointer so your code should read : DoubleWrap operator+(DoubleWrap& x){ DoubleWrap result(this->value + x.value);

answered Mar 21 '15 at 23:49

Roger Alcindor

1

That's not the only mistake in the code. `setParents` is missing a return type, he's passing an object where he'd need to pass a pointer, the `&` after `child` should be before `child`, etc. And those are just the syntax errors. – Emil Laine Mar 21 '15 at 23:52

C++ return by value - what happens with the pointer inside?

2 Answers2

Linked