2

In Item 25 of More Effective C++, Meyer compares this code:

class Widget {
public:
 template<typename T>
 void setName(T&& newName) // newName is
 { name = std::forward<T>(newName); } // universal reference
 …
};

To this code:

class Widget {
public:
 void setName(const std::string& newName) // set from
 { name = newName; }                      // const lvalue
 void setName(std::string&& newName)      // set from
 { name = std::move(newName); }           // rvalue
 …
};

One drawback he outlines with the second snippet vs the first is performance:

With the version of setName taking a universal reference, the string literal "Adela Novak" would be passed to setName, where it would be conveyed to the assignment operator for the std::string inside w. w’s name data member would thus be assigned directly from the string literal; no temporary std::string objects would arise. With the overloaded versions of setName, however, a temporary std::string object would be created for setName’s parameter to bind to, and this temporary std::string would then be moved into w’s data member. A call to setName would thus entail execution of one std::string constructor (to create the temporary), one std::string move assignment operator (to move newName into w.name), and one std::string destructor (to destroy the temporary). That’s almost certainly a more expensive execution sequence than invoking only the std::string assignment operator taking a const char* pointer.

But why can't you just make templates out of them, so the cast to std::string is unnecessary? As in this code, which seems to compile and run just fine with suitable adjustments:

class Widget {
public:
 template<typename T>
 void setName(const T& newName) // set from
 { name = newName; }                      // const lvalue
 template<typename T>
 void setName(T&& newName)      // set from
 { name = std::move(newName); }           // rvalue
 …
};

NB: It's not that I'm arguing for this overloading strategy. Meyer's other arguments are compelling. I would just like some help understanding this particular argument about performance.

Alex Coventry
  • 68,681
  • 4
  • 36
  • 40
  • 1
    Hint: Don't format code using backticks, but indent every code line with four spaces. – πάντα ῥεῖ May 14 '16 at 08:11
  • 3
    Did you somehow swap your second and third snippets? The question as written doesn't make much sense. – T.C. May 14 '16 at 08:15
  • or better yet, just paste your code in, select it, and hit the `{}` button – xaxxon May 14 '16 at 08:26
  • @T.C., yes I did. I've corrected it. Thanks. – Alex Coventry May 14 '16 at 08:30
  • 2
    `T&&` is a forwarding reference, you certainly don't want to unconditionally move from it, because this overload is preferred to `const T&` for non-const lvalues – Piotr Skotnicki May 14 '16 at 08:36
  • 1
    @AlexCoventry why do you think you need two overloads, instead of using the one with `T&&` ? – Piotr Skotnicki May 14 '16 at 08:48
  • @PiotrSkotnicki, I don't. Meyer gives other good arguments for not using the two overloads. I just don't understand the particular performance argument I quoted. – Alex Coventry May 14 '16 at 08:50
  • He gives a potential drawback to the overloading I proposed in the next item: "Functions taking universal references are the greediest functions in C++. They instantiate to create exact matches for almost any type of argument. This is why combining overloading and universal references is almost always a bad idea: the universal refer‐ ence overload vacuums up far more argument types than the developer doing the overloading generally expects." – Alex Coventry May 14 '16 at 08:51
  • 1
    @AlexCoventry in your third snippet you still use this greedy `T&&`, and instead of forwarding you perform a move (which is bad). can you tell the difference between `T&&` and `string&&` ? – Piotr Skotnicki May 14 '16 at 08:53
  • Yes, that's my point. That may be why he didn't consider such code. – Alex Coventry May 14 '16 at 08:57
  • 1
    @AlexCoventry you really should clarify your question. It's still unclear what you don't understand and what is your goal – Piotr Skotnicki May 14 '16 at 09:03
  • @PiotrSkotnicki: Thanks, I can see how my question might seem strange without the context. Meyer had just pointed out that using std::move in the first snippet would be bad, because if setName were called with a local variable n, the subsequent value of n in the calling context is undefined. Then he said you might think the second snippet would be a good way to use std::move and yet avoid that problem. He gave a number of reasons why that's a bad idea, including the one I quoted. It seemed to me that that argument only works because he'd moved from (continued in next comment) – Alex Coventry May 14 '16 at 09:18
  • (from prior comment) because he'd moved from a template function to one with a specific type which is different from the calling argument "Adela Novak". That change is orthogonal to the question of std::move vs std::forward, and makes the argument weak and strange. So I was wondering whether you could correct that problem by switching back to a template function. – Alex Coventry May 14 '16 at 09:21
  • (from prior comment) Not sure how to add that context to the question without burying the key issue. I will take a look at it later. – Alex Coventry May 14 '16 at 09:22
  • 1
    @AlexCoventry *"Meyer had just pointed out that using std::move in the first snippet would be bad"*, yes, that's why he used `forward`, not `move`. hard-coding `string` as the type of parameter means that a raw string literal (or anything convertible to `std::string`) must first be turned in to `std::string` before `setName` is called. that's the performance hit he has in mind. with a forwarding reference you don't need to introduce any more overloads, `T&&` binds to anything, and `forward` correctly restores the value category of an argument expression (this doesn't hold for `move`) – Piotr Skotnicki May 14 '16 at 09:31
  • @PiotrSkotnicki Thanks, that confirms my understanding. – Alex Coventry May 14 '16 at 09:36

2 Answers2

2

(my entire previous answer was terribly wrong and I misinterpreted the code sample I made)

http://melpon.org/wandbox/permlink/d8SAf3FSoo0vw1Di

Because a string literal is actually considered to be an l-value (Why are string literals l-value while all other literals are r-value?), your suggestion ends up calling the lvalue reference version with newName being const char[5] &. However, the name is still directly constructed from the raw string literal data, not resulting in a double constructor call.

Tangentially, if you want to turn the T from a char[5] into a char*, that is what (among other things) std::decay does.

Community
  • 1
  • 1
xaxxon
  • 19,189
  • 5
  • 50
  • 80
  • Thanks for the demonstration. – Alex Coventry May 14 '16 at 08:45
  • why does "fdsa", which is an un-named value (rvalue?) bind to lvalue ref overload rather than the rvalue ref overload of your widget2? – johnbakers May 14 '16 at 08:49
  • 2
    @PiotrSkotnicki keep going, please. – xaxxon May 14 '16 at 08:49
  • @johnbakers I did get confused. http://stackoverflow.com/questions/10004511/why-are-string-literals-l-value-while-all-other-literals-are-r-value – xaxxon May 14 '16 at 08:53
  • that is indeed an unexpected surprise for me as well – johnbakers May 14 '16 at 08:54
  • @PiotrSkotnicki I updated my answer. is it better now? – xaxxon May 14 '16 at 09:00
  • I didn't see the original answer. But I would suggest, if the answer is very different, that you write a new answer. And delete the original if it's not helpful. Now, many of the comments are obsolete – Aaron McDaid May 14 '16 at 09:00
  • @AaronMcDaid it's always available here: http://stackoverflow.com/posts/37224474/revisions – xaxxon May 14 '16 at 09:02
  • I'm not asking to see the original answer. If the answerer says it's wrong, then we don't need to see it. My concern is with the comments. We ask people to submit a new question instead of significantly editing a question, and I take the same approach when I answer – Aaron McDaid May 14 '16 at 09:05
2

This is dangerous and bad code:

class Widget {
public:
  template<typename T>
  void setName(const T& newName) // set from
  { name = newName; }                      // const lvalue
  template<typename T>
  void setName(T&& newName)      // set from
  { name = std::move(newName); }           // rvalue
};

The above has a bug.

Given a variable std::string s, calling setName(s) will move from it.

This is rude and unexpected.

live example.

T&& will bind to both rvalues and lvalues. When you use T&& in a deduced context for T, you should not ever move from it (well almost not ever): you should std::forward<T> from it.

std::forward<T> becomes move iff T&& is an rvalue (a value you should move from).

It is a nearly noop if T&& is an lvalue.

You attempted to split move and copy: you failed. Even if it worked, it would just have been a more verbose version of the perfect forwarding one.

Regardless, all of that template stuff in this case is a bad idea. Just void setName(std::string newName){name = std::move(newName);} for 95% of the performance, half the code, and 1/4 of the complexity.

int main

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524