lvalue vs rvalue dubious

Question

The following code run fine but, for what I understand, it shouldn't

#include <iostream>
#include <vector>

struct Data
{
  explicit Data():value(1){}
  int value;
};

struct Foo
{
  explicit Foo(Data& data):data_(data){}

  inline void print() const
  {
    std::cout<<data_.value<<std::endl;
  }
  Data& data_;
};

void addEntry(std::vector<Foo>& vec)
{
  Data data;
  Foo foo(data);
  vec.push_back(foo);
}

int main()
{
  std::vector<Foo> vec;
  addEntry(vec);
  vec[0].print();
}

The function addEnty create an instance of Data called data. Then creates an instance of Foo, called foo, which stores a reference to data. This istance is then copyed inside the vector vec. Therefore, when the function end, vec[0] should contain a dangling reference since data is destroyed. Am I right? So I would expect to obtain some garbage calling the method print(). Is it only by chance that I obtain the right value 1 or am I missing something?

To make it correct, I would move data in order to avoid the dangling reference. So I would modify the constructor with

explicit Foo(Data&& data):data_(data){}

and the function with

Foo foo(std::move(data));

In this way foo, and consequently its copy inside vec[0], contains the instance data instead of a reference to it. Am I right? Is it the correct solution? In this way, Foo::data_ needs to be of type Data or of type Data&?

score 2 · Answer 1 · answered Aug 20 '15 at 13:48

As you suggest, your sample code has undefined behaviour due to the dangling reference. The behaviour you see is just by chance.

A function taking an rvalue reference says "I'm going to steal data from whatever you pass in". Having a constructor take such a reference is fine so long as those are your semantics, but it doesn't seem like this is the case for your example.

A possibility would be to take the argument by-value, then move this into the member variable:

struct Foo
{
  explicit Foo(Data data):data_(std::move(data)){}
  Data data_;
};

This way, client code can either pass an lvalue (1 copy, 1 move) or an rvalue (2 moves). It's convenient to have only a single constructor to maintain, but this could be inefficient if Data is expensive to move.

Other possibilities would be having a single constructor taking a forwarding reference, or maintaining one overload for rvalues and one for lvalues.

Elohim Meth · Accepted Answer · 2015-08-20T13:56:01.717

2

Yes Foo will hold a dangling reference. Foo class should hold Data not Data& or Data&&.

#include <iostream>
#include <vector>

struct Data
{
  explicit Data():value(1){}
  int value;
};

struct Foo
{
  // this is needed if you want to pass lvalue
  Foo(const Data& data):data_(data) 
  {}
  // for rvalue
  Foo(Data&& data):data_(std::move(data))
  {}

  void print() const
  {
    std::cout<<data_.value<<std::endl;
  }

  Data data_;
};

void addEntry(std::vector<Foo>& vec)
{
  vec.emplace_back(Foo(Data())); 

  // or

  Data data;
  // do somth with data
  vec.emplace_back(Foo(std::move(data)));        

  // or

  Data data;
  // do somth with data
  Foo foo {std::move(data)};
  // do somth with foo, but
  // do not use data here!!!
  vec.push_back(std::move(foo));        
}

int main()
{
  std::vector<Foo> vec;
  addEntry(vec);
  vec[0].print();
}

edited Aug 20 '15 at 13:56

answered Aug 20 '15 at 13:48

Elohim Meth

1,777
9
13

Why do you need to call `std::move` in the inizialization list of constructor `Foo(Data&&)` if you already need to call std::move when passing `data` to construct the object? – Marco Agnese Aug 20 '15 at 14:18
1

Because std::move doesn't move anything anywhere and just converts lvalue to rvalue and even when parameter in move constructor is declared as `Data&&`, expression `data` is lvalue and needs to be converted to rvalue to call a move constructor of `Data` class – Elohim Meth Aug 20 '15 at 14:27
[http://stackoverflow.com/questions/3413470/what-is-stdmove-and-when-should-it-be-used] Take a look here. – Elohim Meth Aug 20 '15 at 14:29
Yes I know that it only convert lvalues to rvalues. But when I call `Foo (std::move(data))` I convert it to an rvalue therefore I don't need `data_(std::move(data))` in the initialization list. When do I need this conversion? It seems to me that it is never used in your code. It is only to understand better these rvalues :) – Marco Agnese Aug 20 '15 at 14:37

Richard Dally · Answer 3 · 2015-08-20T13:55:14.533

You are right, this is working by chance, this actually falls into Undefined Behavior.
Field should be Data type in Foo to avoid dangling reference.
You could rewrite this way:

#include <iostream>
#include <vector>

struct Data
{
  explicit Data():value(1){}
  int value;
};

struct Foo
{
  explicit Foo(Data&& data):data_(std::move(data)){}

  inline void print() const
  {
    std::cout<<data_.value<<std::endl;
  }
  Data data_;
};

void addEntry(std::vector<Foo>& vec)
{
  vec.emplace_back(Foo(Data()));
}

int main()
{
  std::vector<Foo> vec;
  addEntry(vec);
  vec[0].print();
}

lvalue vs rvalue dubious

3 Answers3