Am I using std::move() too often?

Question

I finally feel like I understand move semantics in Modern C++, and it's had a dramatic change on the way I write code. Right now, I'm working on an application that uses dependency injection and I'm incorporating my newfound knowledge of move semantics, but I end up using std::move() so much that I'm worried I'm using it incorrectly.

Previously, if I wanted to inject a dependency that I needed a copy of in my object, I'd write my constructor like this:

class NeedsCopyOfFoo
{
public:
  NeedsCopyOfFoo(const Foo& foo)
    : m_myFoo{foo} {}
private:
  Foo m_myFoo;
};

Now, my classes look like this:

class NeedsCopyOfFoo
{
public:
  NeedsCopyOfFoo(Foo foo)
    : m_myFoo{std::move(foo)} {}
private:
  Foo m_myFoo;
};

There are classes in my design which take as many as three or four class-type dependencies, and I end up moving them all. Obviously, If the caller of my constructor is not able to invoke the constructor with an rvalue, but also isn't going to use the dependency after constructing a NeedsCopyOfFoo object, I also need to use std::move() there, to avoid a completely unnecessary copy.

Is this the way that Modern C++ is supposed to look? Does Uncle Bob mention a code smell of "Uses std::move() too often"? Am I overreacting because I'm just not used to writing in this new style yet?

Answer 1

TL;DR: If you don't care about having perfect performance then

Class(const Foo& foo, const Bar& bar, ...) : m_myFoo{foo}, m_myBar{bar}, ...{...} {}

is the constructor for you. It takes rvalues/lvalues and is going to cost you a copy. It's about as good as you can get and makes life easy, and there is a lot to be said for having an easy life.

---

For just one variable I would have an overload set like

NeedsCopyOfFoo(Foo&& foo) : m_myFoo{std::move(foo)} {}
NeedsCopyOfFoo(const Foo& foo) : m_myFoo{foo} {}

This cost at most one copy or one move operation depending on what type of object is passed to the constructor. This is as perfect as you can get.

Unfortunately this does not scale well. When you start to add more parameter that you want to handle the same way the overload set grows quadratically. That's not fun at all as a 4 parameter constructor would need 16 overloads to be perfect. To combat this we can use a forwarding constructor and limit it with SFINAE so it only takes the types you want. That would give you a constructor like

template<typename T, 
         typename U, 
         std::enable_if_t<std::is_convertible_v<T, Foo> &&
                          std::is_convertible_v<U, Bar>, bool> = true>
Class(T&& foo, U&& bar) : 
    m_myFoo{std::forward<T>(foo)}, 
    m_myBar{std::forward<U>(bar)} {}

This gives you the best performance, but as you can see it is quite verbose and requires you to know a lot more about C++ to work with.

Answer 2

Your code is a gross over-reaction to the newer C++ standards.

Leave these optimisations to the compiler; any decent one will eliminate superfluous value copies and from C++17 are required to do so in some instances.

Effective modern C++ is all about writing less code, not more code.

If you are ever in any doubt that your code is running slowly due to egregious amounts of value copies being taken, then profile it. Profiling is something you should undertake from time to time anyway.