Rvalue references in constructor: when less is more

I've seen a recurring mistake made by well-versed C++03 programmers when they set out to use rvalue references for the first time. In fact, as it turns out, better you are at C++03, easier it is to fall in the trap of rvalue reference anti-pattern I'm gonna talk about.

Consider the following C++03 class:

class Book {
public:
  Book(const std::string & title,
       const std::vector<std::string> & authors,
       const std::string & pub,
       size_t pub_day
       const std::string & pub_month,
       size_t pub_year)
    : _title(title),
      _authors(authors),
      _publisher(pub),
      _pub_day(pub_day),
      _pub_month(pub_month),
      _pub_year(pub_year)
     {}

     // ....
     // ....

private:
  std::string _title;
  std::vector<std::string> _authors;
  std::string _publisher;
  size_t      _pub_day;
  std::string _pub_month;
  size_t      _pub_year;
};

The Book class above is as dull as it can be. Now lets C++11'fy it! C++11 gives us shiny new rvalue references and std::move. So lets add them.

class Book {
public:
  Book(const std::string & title,
       const std::vector<std::string> & authors,
       size_t pub_day
       const std::string & pub_month,
       size_t pub_year)
    : _title    (title),
      _author   (author),
      _pub_day  (pub_day),
      _pub_month(pub_month),
      _pub_year (pub_year)
     {}

  Book(std::string && title,
       std::vector<std::string> && authors,
       size_t      && pub_day
       std::string && pub_month,
       size_t      && pub_year)
    : _title    (std::move(title)),
      _authors  (std::move(authors)),
      _pub_day  (pub_day),
      _pub_month(std::move(pub_month)),
      _pub_year (pub_year)
     {}

     // ....
     // ....

private:
  std::string _title;
  std::vector<std::string> _authors;
  size_t      _pub_day;
  std::string _pub_month;
  size_t      _pub_year;
};

Is our constructor optimally move-enabled? It's far from it! More often that not, programmers' legit code will end up calling the old constructor instead of the the new one, which probably means lost opportunities for optimization. It could be hard to see what's wrong in the new class to an untrained eye. We've test it to see what's really wrong with it.

std::string & toUpper(std::string & s)
{
  std::transform(s.begin(), s.end(), s.begin(), toupper);
  return s;
}
const std::string & January()
{
  static std::string jan("January");
  return jan;
}
int main(void)
{
  std::vector<std::string> authors { "A", "B", "C" };
  Book b1("Book1", authors, 1, "Jan", 2012);   // old c-tor

  size_t year = 2012
  Book b2("Book2", { "A", "B", "C" }, 1, "Jan", year); // old c-tor

  std::string month = "Mar";
  Book b3("Book3", { "Author" }, 1, toUpper(month), 2012); // old c-tor

  Book b4("Book4", { "Author" }, 1, January(), 2012); // old c-tor

  std::string book = "Book";
  Book b5(std::move(book), std::move(authors), 1, std::move(month), year); // old-ctor

  Book b6("Book", { "Author" }, 1, "Jan", 2012); // new c-tor!
}

It may come as a surprise that in all but one case above, the old constructor is called. Only in the last case, the new-ctor is called, which makes the minimum number of copies. So what's the gotcha?

As it turns out, the Book constructors are stopping the compiler from doing a better job. The first constructor takes all the parameters as const reference and the second one takes all the parameters as rvalue reference. Unless and until all the the parameters passed to the constructor are temporary objects (rvalues) or literals, the second constructor does not kick in. This implies lost optimization opportunities for parameters that are temporary (so can be moved) but won't be moved because some other parameter botched the soup. These two constructors give you very limited options: all-or-nothing.

In case of b1, authors is an lvalue and it does not bind to an rvalue ref.
In case of b2, year is an lvalue and does not bind to an rvalue ref.
In case of b3, toUpper returns a string reference; Does no good.
In case of b4, January returns a const string reference; same story!
In case of b5, year is an lvalue that prevents calling the second constructor although programmer tries to explicitly move all the string and vector parameters. In reality, the actual moves do not happen and if the remaining program depends on the moves being successful may not be very happy.

Only in case of b6, all actual parameters are rvalues or literals. Therefore, the "all-rvalue-ref" constructor is called. Note that temporary string objects will be implicitly created where string literals are used.

So lets fix it. What we really need is just one constructor that accepts all the parameters by value. As a side-effect, the overall code is much simpler.

class Book {
public:
  Book(std::string title,
       std::vector<std::string> authors,
       size_t      pub_day
       std::string pub_month,
       size_t      pub_year)
    : _title    (std::move(title)),
      _authors  (std::move(authors)),
      _pub_day  (pub_day),
      _pub_month(std::move(pub_month)),
      _pub_year (pub_year)
     {}

  // ....
  // ....
};

That's it! This is our the constructor that works optimally. All strings, vectors, integral types are passed by value. Within the constructor, the pass-by-value parameters that are on the stack-frame must be moved to the respective data members. The reason is that the lifetime of pass-by-value parameters is anyways limited to the lifetime of the constructor call itself. We want to tie their lifetime to the object and not the constructor.

This constructor does no more deep copies of than necessary. And that number really depends on how it is called. Due to pass-by-value semantics, each object individually is an opportunity for the compiler to perform a move when a temporary is involved. Lets revisit our b1 to b4 objects.

In case of b1, except for authors all other parameters are copied and moved once.
In case of b2, all strings and vectors are copied and moved once. That's what matters.
In case of b3, toUpper returns an string reference; everything else copied and moved only once.
In case of b4, January returns a const string reference; everything else copied and moved once.
In case of b5, strings are vector are moved as expected and in fact the b5 object the local parameters are created using move-constructor and moved again into the data member. Hence the object is created without any deep copies (like zero-copy).

Using pass-by-value also opens other opportunities to eliminate temporaries when functions return by value and are used as actual parameters. However, I won't discuss that in detail here.

Finally, I want to conclude saying that this whole thing assumes that a move is much cheaper than a deep-copy. This is generally true for std::vector and std::string where memory is allocated dynamically. For small strings however small-string-optimization may make copy and move practically equivalent.