10

Let's say I have:

template<class T>
struct NodeBase
{
    T value;
    NodeBase(T &&value)
        : value(value) { }
};

and I inherit from it:

template<class T>
struct Node : public NodeBase<T>
{
    Node(T &&value)
        : NodeBase( WHAT_GOES_HERE (value)) { }
};

Should WHAT_GOES_HERE be std::move or std::forward<T>? Why?

user541686
  • 205,094
  • 128
  • 528
  • 886
  • I definitely had to stop and think for a second to be certain of the answer in my head. – Mooing Duck Jun 28 '13 at 00:48
  • HAHA! Since all the answers were deleted, we [discovered that the constructor _is_ doing reference collapsing](http://coliru.stacked-crooked.com/view?id=d69df4771b7c91e4eec3e82b32465d89-803889f054654ad4b92ce24ea171578e), and you _can_ pass lvalue references to it. – Mooing Duck Jun 28 '13 at 01:06
  • Doing reference collapsing is easy: `typedef int& T; T&& x;`, tadah – Andrew Tomazos Jun 28 '13 at 01:12
  • 1
    @Mehrdad What behavior do you want? You could put nothing there, and it would result in *some* behavior. What does `Node` *mean*, and from what kind of objects do you want to be able to construct it? How about `Node`, `Node`, `Node` and `Node`? – Yakk - Adam Nevraumont Jun 28 '13 at 02:44
  • 1
    @MooingDuck of course: universal references do not require a type deduction context. They are just relatively useless outside of a type deduction context! In a type deduction / perfect forwarding context, the behavior you want is relatively obvious: here, it is less so. – Yakk - Adam Nevraumont Jun 28 '13 at 02:45
  • @Yakk: I want the user to be able to store whatever he wants inside that node... the user might want to put a reference, or a pointer, or a plain object; I don't want to add unnecessary restrictions on what the user can put in there if I can avoid it. – user541686 Jun 28 '13 at 02:49
  • @Mehrdad: Why does `NodeBase` accept only an rvalue reference and then copy from it? That seems counterintuitive doesn't it? – Mooing Duck Jun 28 '13 at 03:56
  • @MooingDuck: If `T` isn't a reference, then the user is asking for it to be copied... and if `T` is a reference then the user is asking for it not to be copied. Both of those seem to lead to the intended scenarios... I might have messed up, but I don't know how. What am I missing? – user541686 Jun 28 '13 at 04:09
  • If it's an lvalue reference it should be copied yes, but rarely does one copy an rvalue reference, and `NodeBase` _only_ accepts rvalue references. – Mooing Duck Jun 28 '13 at 04:33
  • @MooingDuck: Er... if `T` is `int &`, then doesn't `NodeBase` accept `int &`? Where's the r-value reference you're saying is only accepted? I feel like I'm not understanding what you mean... – user541686 Jun 28 '13 at 04:40
  • I was thinking about the case where `T` is a `std::string`. In that case, `NodeBase` only accepts `std::string&&`, and then takes a copy of it. Right? – Mooing Duck Jun 28 '13 at 04:47
  • @MooingDuck: Yeah, isn't that supposed to happen? The user didn't specify a reference type, so that means he wants it to be copied... – user541686 Jun 28 '13 at 04:59
  • No, if I have a Node that contains a `std::string`, the two options that make sense are to copy a string into the node, or to _move_ a string into the node. Usually, if you're going to copy, you take by `const T&`, if you're going to move, you take by `T&&`. Your code takes by `T&&`, implying you're going to move the string. – Mooing Duck Jun 28 '13 at 16:15
  • @MooingDuck: So what are you saying it should be instead? – user541686 Jun 28 '13 at 17:05
  • I think you should (A) have two: `T&&`(moved) and `const T&`(copy), which is always optimal; (B) have one: `T`(moved), which is usually almost optimial; or (C) `template U&&`(perfect forwarding) which is always optimal, _and has awesome effects_. See Yakk's answer. – Mooing Duck Jun 28 '13 at 17:37

3 Answers3

6

Since in the implementation of the constructor of Node<T> it is unknown whether T is a plain type (i.e. not a reference), or a reference,

std::forward<T>(value)

is suitable.

std::forward<T>(value) is the right choice whenever it isn't known whether T && binds to an rvalue or an lvalue. This is the case here because in the constructor we don't know whether T && is equivalent to U && for some plain type U, or equivalent to U & &&.

It doesn't matter whether T is deduced in the function call that uses std::forward or determined at a different time (such as in your example, where T is determined at the time when the Node template is instantiated).

std::forward<T>(value) will call the inherited constructor in the same way as if the base class constructor had been called directly by the caller. I.e., it will call it on an lvalue when value is an lvalue, and on an rvalue when value is an rvalue.

jogojapan
  • 68,383
  • 11
  • 101
  • 131
  • But you're right that `std::forward` does seem to be the best option: http://coliru.stacked-crooked.com/view?id=b6c7ec72b3a390dec59228e1f5e4ccd1-803889f054654ad4b92ce24ea171578e. (The noted errors are expected, merely experiments) – Mooing Duck Jun 28 '13 at 16:21
  • `U & &&` _is_ `U&` because of reference collapsing. – Mooing Duck Jun 28 '13 at 16:23
  • @MooingDuck Yes, sure. I have written it as `U & &&` to emphasize that it's composed of `T = U &` (the declared type) and `&&` (from the function parameter list). – jogojapan Jun 29 '13 at 09:29
4

Probably neither.

What I suspect you should have is:

template<class T>
struct NodeBase
{
  T value;
  NodeBase(NodeBase &&) = default;
  NodeBase(NodeBase const&) = default; // issue: this might not work with a `T&`, but we can conditionally exclude it through some more fancy footwork
  NodeBase(NodeBase &) = default;
  template<typename U, typename=typename std:enable_if< std::is_convertible<U&&, T>::value >::type >
  NodeBase(U &&u)
    : value(std::forward<U>(u)) { }
};

template<class T>
struct Node : public NodeBase<T>
{
  Node( Node & ) = default;
  Node( Node const& ) = default; // issue: this might not work with a `T&`, but we can conditionally exclude it through some more fancy footwork
  Node( Node && ) = default;
  template<typename U, typename=typename std:enable_if< std::is_convertible<U&&, NodeBase<T>>::value >::type>
  Node(U && u)
    : NodeBase( std::forward<U>(u) ) { }
};

unless you are doing something exceedingly strange.

By exceedingly strange, it means that if your T is an int, you want to only accept moved-from values into your Node, but if your T is an int&, you accept only non-const lvalue ints, and if T is an int const&, you accept any value convertible to int.

This would be a strange set of requirements to place on the constructor of NodeBase. I can think of situations where this might be the case, but they are not common.

Assuming you simply want NodeBase to store that data, taking T&& in the constructor is not the right thing to do -- if you are storing an int in NodeBase, you probably are willing to make copies of that int, instead of only accepting moved-from ints.

The above code does exactly that -- it allows anything that could be stored in the NodeBase to be passed on up to said NodeBase, with perfect forwarding.

On the other hand, if you actually want the strange set of construction restrictions, then this is not the right answer for you. I've used that when I was building the a template type that was built from a universal reference argument, and I did want to restrict the passed in type to match the universal reference argument exactly, and store it iff the argument was an rvalue reference, and otherwise keep a reference to it.

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
  • It may or may not be the answer sought for by the question, but it's a good answer to have here for anyone in a similar but more standard situation. +1. – jogojapan Jun 28 '13 at 02:37
  • +1 it's making me double-check what I wrote, you might very well be right! – user541686 Jun 28 '13 at 04:10
1

T isn't deduced in your example. The T is a class template parameter, not a function template parameter. So assuming you will not use a reference type as T, T&& will be an rvalue-reference to T, so it will only bind to rvalues of type T. these will be safe to move so you can use std::move here.

template<class T>
struct Node : public NodeBase<T>
{
    Node(T &&value)
        : NodeBase( std::move (value)) { }
};

int main()
{
    int i = 3;

    Node<int> x(42); // ok
    Node<int> y(std::move(i)); // ok
    Node<int> z(i); // error
}

std::forward is normally only for places where you have a deduced type that may either be an lvalue-reference or rvalue-reference.

template<class T>
void f(T&& x)
{
    ... std::forward<T>(x) ...
}

Because T&& may actually be either an lvalue-reference or rvalue-reference. This is only because T is deduced in this context.

Andrew Tomazos
  • 66,139
  • 40
  • 186
  • 319
  • When I try `template struct MyStruct { MyStruct(T &&) { } }; int main() { int x = 5; MyStruct m(x); }` in Visual C++, it collapses references just fine. Is the compiler wrong, or this answer? :P – user541686 Jun 28 '13 at 01:08
  • @Mehrdad: That is correct, normally you wouldn't use reference types as template arguments. If that is the usage of your class, then you should use std::forward to preserve the reference category. – Andrew Tomazos Jun 28 '13 at 01:08
  • If he uses `std::forward` then it breaks for non-reference types. @Mehrdad: Rule of thumb: don't take references in template parameters unless that is the _sole purpose_ of the class. – Mooing Duck Jun 28 '13 at 01:15
  • I've lost track of exactly what we are trying to do here. In the OP code we are storing a value of type `T`, and passing through a reference as a constructor parameter. What do you want the behaviour to be for the different type arguments to `T`? – Andrew Tomazos Jun 28 '13 at 01:19
  • @MooingDuck You say that if `std::forward` is used, it breaks for non-reference types. Can you explain that? What exactly breaks? – jogojapan Jun 28 '13 at 01:39
  • [No matching function call](http://coliru.stacked-crooked.com/view?id=babcadd21b724638f845d5a7b9a97d2c-4f34a5fd633ef9f45cb08f8e23efae0a) – Mooing Duck Jun 28 '13 at 02:07
  • @MooingDuck Yes, but this is because `T` is `std::string`, hence the only available constructor is `NodeBase(std::string &&)`. The only thing you are allowed to pass to this is a `std::string` rvalue (which `s` is not). But that's by design -- because there is only this one constructor. It's not `std::forward`'s fault. On the contrary, it would be a bug if `std::forward` were to make this error disappear. – jogojapan Jun 28 '13 at 02:20
  • That's correct: [Moved...](http://coliru.stacked-crooked.com/view?id=d2739cdd6c2076334ecbb19b9d247611-f674c1a6d04c632b71a62362c0ccfc51). Where you could also use foo() to trigger the constructor properly. – Pixelchemist Jun 28 '13 at 02:34