std::move or std::forward when assigning universal constructor to member variable in C++

Question

Consider the following classes foo1 and foo2

template <typename T>
struct foo1
{
    T t_;

    foo1(T&& t) :
        t_{ std::move(t) }
    {
    }
};

template <typename T>
struct foo2
{
    foo1<T> t_;

    foo2(T&& t) :
        t_{ std::forward<T>(t) }
    {
    }
};

Is it always the case that the constructor of foo1 represents the correct way to initialise the member variable T? i.e. by using std::move.

Is it always the case that the constructor of foo2 represents the correct way to initialise the member variable foo1<T> due to needing to forward to foo1's constructor? i.e. by using std::forward.

Update

The following example fails for foo1 using std::move:

template <typename T>
foo1<T> make_foo1(T&& t)
{
    return{ std::forward<T>(t) };
}

struct bah {};

int main()
{
    bah b;

    make_foo1(b); // compiler error as std::move cannot be used on reference

    return EXIT_SUCCESS;
}

Which is a problem as I want T to be both a reference type and a value type.

Answer 1

Neither of these examples use universal references (forwarding references, as they are now called).

Forwarding references are only formed in the presence of type deduction, but T&& in the constructors for foo1 and foo2 is not deduced, so it's just an rvalue reference.

Since both are rvalue references, you should use std::move on both.

If you want to use forwarding references, you should make the constructors have a deduced template argument:

template <typename T>
struct foo1
{
    T t_;

    template <typename U>
    foo1(U&& u) :
        t_{ std::forward<U>(u) }
    {
    }
};

template <typename T>
struct foo2
{
    foo1<T> t_;

    template <typename U>
    foo2(U&& u) :
        t_{ std::forward<U>(u) }
    {
    }
};

You should not use std::move in foo1 in this case, as client code could pass an lvalue and have the object invalidated silently:

std::vector<int> v {0,1,2};
foo1<std::vector<int>> foo = v;
std::cout << v[2]; //yay, undefined behaviour

A simpler approach would be to take by value and unconditionally std::move into the storage:

template <typename T>
struct foo1
{
    T t_;

    foo1(T t) :
        t_{ std::move(t) }
    {
    }
};

template <typename T>
struct foo2
{
    foo1<T> t_;

    foo2(T t) :
        t_{ std::move(t) }
    {
    }
};

For the perfect forwarding version:

• Passed lvalue -> one copy
• Passed rvalue -> one move

For the pass by value and move version:

• Passed lvalue -> one copy, one move
• Passed rvalue -> two moves

Consider how performant this code needs to be and how much it'll need to be changed and maintained, and choose an option based on that.

Answer 2

This depends on how you deduce T. For example:

template<class T>
foo1<T> make_foo1( T&& t ) {
  return std::forward<T>(t);
}

In this case, the T in foo1<T> is a forwarding reference, and your code won't compile.

std::vector<int> bob{1,2,3};
auto foo = make_foo1(bob);

the above code silently moved from bob into a std::vector<int>& within the constructor to foo1<std::vector<int>&>.

Doing the same with foo2 would work. You'd get a foo2<std::vector<int>&>, and it would hold a reference to bob.

When you write a template, you must consider what it means for the type T to be reference. If your code doesn't support it being a reference, consider static_assert or SFINAE to block that case.

template <typename T>
struct foo1 {
  static_assert(!std::is_reference<T>{});
  T t_;

  foo1(T&& t) :
    t_{ std::move(t) }
  {
  }
};

Now this code generates a reasonable error message.

You might think the existing error message was ok, but it was only ok because we moved into a T.

template <typename T>
struct foo1 {
  static_assert(!std::is_reference<T>{});

  foo1(T&& t)
  {
    auto internal_t = std::move(t);
  }
};

here only the static_assert ensured that our T&& was actual an rvalue.

---

But enough with this theoretical list of problems. You have a concrete one.

In the end this is probably want you want:

template <class T> // typename is too many letters
struct foo1 {
  static_assert(!std::is_reference<T>{});
  T t_;

  template<class U,
    class dU=std::decay_t<U>, // or remove ref and cv
    // SFINAE guard required for all reasonable 1-argument forwarding
    // reference constructors:
    std::enable_if_t<
      !std::is_same<dU, foo1>{} && // does not apply to `foo1` itself
      std::is_convertible<U, T> // fail early, instead of in body
    ,int> = 0
  >
  foo1(U&& u):
    t_(std::forward<U>(u))
  {}
  // explicitly default special member functions:
  foo1()=default;
  foo1(foo1 const&)=default;
  foo1(foo1 &&)=default;
  foo1& operator=(foo1 const&)=default;
  foo1& operator=(foo1 &&)=default;
};

or, the simpler case that is just as good in 99/100 cases:

template <class T>
struct foo1 {
  static_assert(!std::is_reference<T>{});
  T t_;

  foo1(T t) :
    t_{ std::move(t) }
  {}
  // default special member functions, just because I never ever
  // want to have to memorize the rules that makes them not exist
  // or exist based on what other code I have written:
  foo1()=default;
  foo1(foo1 const&)=default;
  foo1(foo1 &&)=default;
  foo1& operator=(foo1 const&)=default;
  foo1& operator=(foo1 &&)=default;
};

As a general rule, this simpler technique results in exactly 1 move more than the perfect forwarding technique, in exchange for a huge amount less code and complexity. And it permits {} initialization of the T t argument to your constructor, which is nice.