CODEWITHSUNDEEP
c++c++11movelvaluervalue
Marco Agnese
Marco Agnese

Reputation: 369

lvalue vs rvalue dubious

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&?

Upvotes: 1

Views: 95

Answers (3)

Elohim Meth
Elohim Meth

Reputation: 1827

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();
}

Upvotes: 2

Richard Dally
Richard Dally

Reputation: 1450

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();
}

Upvotes: 1

TartanLlama
TartanLlama

Reputation: 65620

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.

Upvotes: 2

Related Questions