

Object Theft Etiquette in C++: methods with a side of && - quicknir
https://turingtester.wordpress.com/2015/07/05/object-theft-etiquette-in-c-methods-with-a-side-of/

======
wallstop
This post horrified me. This may be due to two factors: (1) having not written
any serious C++ in the past year and (2) a focus on "correct" or "easily
verifiable" code.

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.

~~~
lultimouomo
Your horror is completely justified: this is not correct C++. Accessing an
object after it has been move()d it squarely undefined behaviour

~~~
Matheus28
It is not undefined behaviour. The object is left in a valid, but unspecified
state. If you .clear() it or whatever (in case of a vector), you'll have a
perfectly good object to use.

~~~
lultimouomo
You're right; s/undefined/unspecified/.

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)

~~~
quicknir
Actually, that's not true. Here's what I wrote:

"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.

~~~
wallstop
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](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)

~~~
SamReidHughes
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](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.

------
stinos
Calling moving objects 'stealing', calling for 'dishonest citizens' etc gives
moving a bad connotation which it hardly deserves at all, especially not in
correct code. The main reason is that the object which is moved from is
usually not going to be used anymore anyway so it doesn't matter what happens
to it's content. That is just how it is meant to be used by the folks
designing it: transferring content. Calling that stealing would be like
calling the righteous transport of goods from one warehouse to another
warehouse stealing: it is not, all involved parties know that after the
transaction the first warehouse will be empty and the latter is full. The
premise set in this article (first move, then use the object again and expect
it to have content) goes against this. Which is actually fine, unless of
course you expect that moving somehow leaves the original intact. Which it
doesn't, that is what copying is supposed to be used for. At least in my
opinion.

~~~
quicknir
Dude, the stealing etc was clearly tongue in cheek.

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.

------
SamReidHughes
There is no particular reason for the 'std::string&& str() &&' method to
return a std::string&&. You could return a std::string instead, and still
avoid copying. Sometimes you'll get an extra move construction, where it isn't
elided on the return side of things, but your code will be more resistant to
bad refactorings. The member variable will always be moved from, instead of
having that depend on how the caller behaves. Also, it's less likely that
you'll code your way into returning an invalid reference.

~~~
detrino
This kind of interface has precedent:
[http://en.cppreference.com/w/cpp/experimental/optional/opera...](http://en.cppreference.com/w/cpp/experimental/optional/operator*)

~~~
SamReidHughes
I'm not saying the interface is externally bad. The danger and reasoning here
is that the implementation could be made bad. STL functions can tolerate that
risk, and they don't have the luxury of knowing the object's a std::string on
an uncritical path, not some graph node with zillions of back-pointers. (Edit:
I think their interface also avoids copying when dealing with non-movable
types.)

------
ridiculous_fish
It looks like `m_cached = true;` was accidentally omitted. As written it is
always false.

~~~
quicknir
Thanks for the catch, fixed.

