Object Theft Etiquette in C++: methods with a side of &&
turingtester.wordpress.com
turingtester.wordpress.com
I can understand the urge to want to have the speed and power of move, but it feels like you're destructively working around the standard. The proposed API in SetStringer seems very dangerous; you're passing back a const ref to a string that isn't really const at all (successive calls after the set has been updated will mutate it's state, users may be unaware of this).
Move, copy, and (const) references each have their place and should be used accordingly. Move when you want a transfer of ownership, copy when you want duplication (at this instance) of ownership to another owner, ref when you want shared ownership. The intent of the article seems to actively go against these intents; stinos' comment sums up the rest of my feelings perfectly.
An accessor returning a const reference is perfectly normal. Something being const in C++ never meant that other things can't modify it, and the risk you describe has nothing to do with it being a const reference, as opposed to the non-const kind. For example, one given by the article, in fact, you get the same situation with an even bigger risk profile with the expression v[i], where v is a std::vector<std::string>.
> ref when you want shared ownership.
Not at all. If you have a reference to something, you don't own it.
The comment on the article still stands though. You're still invoking unspecified behaviour, and this is a horrible practice - it forces you to take not on which classes you are sure of the actual behaviour when accessing after move, and you're bound to slip.
(Note that the article itself uncorrectly says that move leaves string in an invalid state, which is what threw me off track)
"By stealing the string, we put foo in an invalid state".
foo is an object of the SetStringer class. It puts it an in invalid state because it no longer maintains its own invariants.
The whole point of this article is about how you can move member variables for efficiency, while ensuring the owning object is left in a valid state (as move/rvalue semantics require).
Wanting to be able to move a member variable is no stranger than wanting to move the object itself; in that sense an rvalue ref overloaded getter is no stranger than move construction/assignment, it's just more rare because it's less used.
While I am not a C++ day-developer nor guru, from what I have seen and written of modern C++, the "move member variables" is not "good", in any sense of the word, stemming from the reasons put forth by many a comment.
If you ever want to move a member variable, I would highly recommend that you re-think your design. Ask questions such as "why do I want this?" and "is it really more efficient?". In some cases, where perf is really that important, then yea, go for it. However, just because something can be done, does not mean that it should be done.
While I still disagree with your const & return of the original string, I threw together an example showing the same exact API you proposed, but without moving member variables:
https://gist.github.com/wallstop/4d80fba9b1d15da64488
Depending on method usage, this may or may not invoke the cost of an extra string creation (+ extra on move method, - for continual access of cached string)
Generally speaking, there's nothing wrong at all with moving from a member variable. You can see an example of where it's perfectly all right in this article.
> you have some cached string that you may or may not want to move out. If you move it, the next time you access "it", the string needs to be re-computed. This means the string is going to re-malloc the memory that it had before it was moved. I don't understand what "efficiency" the outlined code gives you.
The method in question can only be called on an rvalue reference, which means the object, in its present state, is not going to be used again in the future anyway, so the problem of reallocating the string is moot.
> https://gist.github.com/wallstop/4d80fba9b1d15da64488
Your code has a use-after-free bug, because your std::string&& str() && function returns a local variable by reference. Your 'std::string&& str() &&' function doesn't even destructively modify the object, so there's no reason for it to be marked '&&' in the first place, and there's no reason for it to exist at all, because its performance is worse than the 'const std::string & str() &' version.
If you want a concrete example, consider std::stringstream. You agree this class is useful, right? In many cases, you build up a string, bit by bit. You then want to extract the string at the end. Because stringstream was written before move semantics, that string is returned by value. That means a full copy of the internal string buffer is made.
In 95% of real life use cases of stringstream, the stringstream is a local variable used to build up a string and then discarded. So copying that string is fundamentally a waste. But exposing the string buffer in a way that std::move could be fruitfully called on it is dangerous, it may break stringstream's class invariants and turn it into a toxic object, this is avoided in C++. So the preferred solution would be to give stringstream::str an rvalue overload, so that the stringstream can safely release its string buffer. Does that make sense?
Of course I am aware that this code involves extra work, is harder for less experienced team members to understand, etc. I wouldn't do it in the vast majority of situations. But sometimes it is useful. The goal was to try to help people understand &&/& member overloads, as there isn't much discussion of them.
Respectfully, I am a professional c++ developer. Odds are that the code I write is under greater pressure for performance and genericity. Maybe a bit of benefit of the doubt is in order.
#include <string>
#include <stdio.h>
struct A {
const std::string& str() const & { return m_str; }
std::string&& str() && { return std::move(m_str); }
std::string m_str;
};
void func(const std::string& s) { puts("Got const value"); }
void func(std::string&& s) { puts("Got rvalue"); }
A returns_a() { A a; return a; }
int main()
{
func(returns_a().str());
A b;
func(b.str());
return 0;
}It seems like the example I picked was a bit unfortunate in the following sense: a couple of people have already misinterpreted the point of the article to be that foo still had content after I moved its string (I won't use stole anymore, seems like a touchy subject).
That's not the point, the point is that the invariants are maintained, which is the requirement for a valid state. I could have reset m_set at the same time as I set m_cached to false, it would still be just as correct.
Edit: I have modified the post slightly so that it's a bit clearer that what I'm after is a valid state for foo, not to maintain the specific things that were inserted.