10

I find these mistake to be all-too-easy for developers to make. Is there a best practice or authoritative way to avoid this? Is there a compiler flag or hint that works across multiple platforms?

Darkenor
  • 4,349
  • 8
  • 40
  • 67
  • I don't know of any compile-time system for this. I assume some static analyzers do it. In my experience a runtime `assert` is enough for most cases. Edit : Note that different types have different guaranties regarding their post-move state. Many standard types are perfectly fine to use, but have had their values set to some known "empty" state. So it might be harder than you think to implement such a check. – François Andrieux Mar 13 '19 at 14:01
  • 4
    Easy rule: Don't let them use `std::move`, and if absolutely they have to, put that variable in a scope so it will not be available outside of the scope it was needed and can't be used. – NathanOliver Mar 13 '19 at 14:05
  • 4
    Using a static analyzer like clang-tidy can help you in finding problematic parts of the code. ([clang-tidy: bugprone-use-after-move](https://clang.llvm.org/extra/clang-tidy/checks/bugprone-use-after-move.html)). – t.niese Mar 13 '19 at 14:05
  • The tool does not need to be cross platform, but you should make it available/enforced in your code reviewing process, as part of e.g. your CI or pull request. – t.niese Mar 13 '19 at 14:07
  • 4
    Exactly what do you mean by "use after move" mistake? Reading from a moved-from object? Do you have problems with own types, or `std`? If own types, you can put the object into a state, which can be asserted on afterwards. – geza Mar 13 '19 at 14:09
  • 1
    Another option is to not allow `std::move` at all outside of member initialization lists or lambda captures. If you have a case where you need to create an object, do some stuff to it, and then move it off somewhere, you can easily encapsulate that into a function or lambda and have the value returned to the thing it needs to move to and since it will be an rvalue the move will happen automatically. Not sure if this is actually good advice, but it might be worth looking into. – NathanOliver Mar 13 '19 at 14:14
  • 2
    Generally, I don't think there's anything wrong with _use-after-move_. Consider common generic swap, such as: `template void swap(T& a, T& b) { T c(std::move(a)); a = std::move(b); b = std::move(c); }` ([e.g., libstdc++ impl](https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/bits/move.h#L193)). First, `a` is moved-from and then its move/copy assignment operator is invoked (i.e. `a` is used after move). Do you want the tool you are looking for to warn you about every swap? – Daniel Langr Mar 13 '19 at 14:21
  • 3
    @DanielLangr That's not really a use after move, at least not what is normaly referre to if you talk about user after move, you re assign `a` after you moved from it, but you do not use it after move. A `a.foo()` would be a user after move. – t.niese Mar 13 '19 at 14:29
  • 1
    Are you asking about `std::` types which only promise "valid but unspecified state" after being moved-from? Or other libraries' types where moved-from objects may be in invalid states? Or your own types? – Caleth Mar 13 '19 at 14:30
  • @t.niese what's the `=`, if not a use? remember it is also expressible as `a.operator=(std::move(b));` – Caleth Mar 13 '19 at 14:30
  • @t.niese IIRC, you are also allowed to call some member functions for `std::` objects in unspecified state. Such as `std::vector::capacity` or `::size`. See, e.g., this answer: https://stackoverflow.com/a/9168917/580083. – Daniel Langr Mar 13 '19 at 14:33
  • 1
    @DanielLangr That's true. But if a class is move assignable and it compiles, then it should be valid to be used (or the class design is broken). But for a `a.foo()` the user needs to look at the documentation, if it is valid to call the function (if it does not have any preconditions). So for a correctly implemented class the compiler should be able to tell you if `a = std::move(b);` is valid. – t.niese Mar 13 '19 at 14:40
  • "find these mistake to be all-too-easy for developers to make". I personally don't think so. It's way much more easy to get a dangling reference or work with an expired temporary. Personal opinion. And that doesn't mean it's not interesting to find a solution. – bolov Mar 13 '19 at 14:46
  • @t.niese The same could be said of default constructors. `std::vector ints; ints[0];` is undefined behaviour. Should we ban default constructors? or `[]`? The thing about "valid but unspecified state" is that you can call *exactly only* the members with no preconditions safely. – Caleth Mar 13 '19 at 14:46
  • what is "use after move"? how can it be a mistake? Where is your [MCVE] ? – 2785528 Mar 13 '19 at 16:10

3 Answers3

3

An effective rule of thumb: Never use std::move nor std::forward and never type cast to an rvalue (or universal) reference. If you never move from a variable or a reference, then you can't make the mistake of using it after. This approach obviously has a drawback, since those utilities are useful in some cases for turning a copy into a move when the move is sufficient; not to mention the cases where it is necessary.

Approach for your own types: Add assertions into member functions that verify whether the instance has been moved from, and rely on them to trigger during testing. The "moved" state will need to be stored as a member. Assertions and the member can be removed in release build. A drawback is that this adds otherwise unnecessary boilerplate to every member function.

An extra approach: Use a static analysis tool that attempts to detect the potential mistake.

A reasonable rule of thumb: Keep your functions short. When the function is short, a use will be close to the move, and thus the potential mistake is easier to spot.

eerorika
  • 232,697
  • 12
  • 197
  • 326
2

So the issue really is "read" after move. I think I agree that any use of std::move should be code reviewed as a potential risk. If the std::move is at the end of a function with a local value or value parameter, all good.

Anything else needs scrutiny, and any use of the variable after the move should be given attention. I guess giving the variable an "_movable" suffix will also help with the code review.

The few cases of write after move, such as swap will just have to be defended during the review.

Personally, I still treat std::move as a smell in the code, much like casts.

I'm not sure of any lint rules that would enforce this pattern, but I'm sure they are easy to write :-)

Gem Taylor
  • 5,381
  • 1
  • 9
  • 27
2

To ban std::move is short-sighted. The best way to avoid read-after-move is to std::move at the end of the scope that introduced the moved-from variables.

j6t
  • 9,150
  • 1
  • 15
  • 35