Comment by wallstop | Hacker News Reader

Comment by wallstop | Hacker News Reader

Parent Full thread

That's my main point of disagreement: wanting to move a member variable is more strange than moving the object itself. Consider your use case: 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.

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)

His example is perfectly fine. The example that puts the object in an invalid state is an example of what not to do. The example of what you should do, which is to make the method marked "&&" so it can only be used on rvalue reference, i.e. an object that's going to be discarded anyway, makes a method that leaves the object in a valid state and one that conforms perfectly to expectations. It even politely cleans up m_set.

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.

So, in retrospect, my example was not a good one, I'm afraid. the recomputing of the string was a red herring. All that matters is that the moved-from object was in a valid state. You can look at the updated post, maybe it will help a bit.

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.

Reply on news.ycombinator.com