5 ms·

Object Theft Etiquette in C++: methods with a side of &&

21 points by quicknir 11y ago

stinos 11y ago
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.
- enqk 11y ago
  I always think of the X in std::move(X) as an object that is about to be destructed. I.e. treat a bit the std::move cast as a marker for destruction
- quicknir 11y ago
  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.
ridiculous_fish 11y ago
It looks like `m_cached = true;` was accidentally omitted. As written it is always false.
- quicknir 11y ago
  Thanks for the catch, fixed.
wallstop 11y ago
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 11y ago
  Your horror is completely justified: this is not correct C++. Accessing an object after it has been move()d it squarely undefined behaviour
  - Matheus28 11y ago
    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 11y ago
      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 11y ago
SamReidHughes 11y ago
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 11y ago
  This kind of interface has precedent: http://en.cppreference.com/w/cpp/experimental/optional/operator* http://en.cppreference.com/w/cpp/experimental/optional/opera...
  - SamReidHughes 11y ago
    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.)

21 points by quicknir 11y ago

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.

enqk 11y ago
I always think of the X in std::move(X) as an object that is about to be destructed. I.e. treat a bit the std::move cast as a marker for destruction
quicknir 11y ago
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.

ridiculous_fish 11y ago

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

quicknir 11y ago
Thanks for the catch, fixed.

wallstop 11y ago

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 11y ago
Your horror is completely justified: this is not correct C++. Accessing an object after it has been move()d it squarely undefined behaviour
- Matheus28 11y ago
  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 11y ago
    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 11y ago

SamReidHughes 11y ago

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 11y ago
This kind of interface has precedent: http://en.cppreference.com/w/cpp/experimental/optional/operator* http://en.cppreference.com/w/cpp/experimental/optional/opera...
- SamReidHughes 11y ago
  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.)