Why not allow Reset() after std::move

[Christian Biesinger]

Hello,

this just came up in a code review -- there is a rule here:

https://www.chromium.org/rvalue-references/#5-you-must-not-use-a-variable-after-calling-stdmove-on-it

that operator= is allowed after std::move. But it seems like calling a Reset() method should be allowed too, since that would also put the object in a defined state.

What do you think? (In the specific case this was a base::Callback)

Christian

[Daniel Cheng]

Send a CL to annotate Reset() with REINITIALIZES_AFTER_MOVE. We do that for a number of other things.

Daniel

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAPTJ0XH8EHjsOGzji%2Bmf9wgdHWYFY9NFCCrSkPzWjdQc4VNBgQ%40mail.gmail.com.

[K. Moon]

Isn't the guidance in this document a little too strong in the first place? There are a number of types for which std::move() leaves the type in a well-specified state, such as std::unique_ptr becoming nullptr, base::OnceCallback becoming null, std::vector becoming empty, tc. It's safer not to assume anything about the moved-from variable in most cases, but this is a rule of thumb, not a strict rule.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKqYiujp4XdYAnniY%3DV9jROcAt3duYyk51HBnmhVzqCUOQ%40mail.gmail.com.

[John Admanski]

Most recommendations against touching moved-from objects tend to be overly strict. They tend to gloss over the fact that objects often have methods that are perfectly safe to call after a move (especially stuff like a "reset" or "clear" that unconditionally puts the object into a known state) and also the fact that there are classes (like std::unique_ptr that you mentioned) that explicitly define what their moved-from state will be.

In general this is probably reasonable for a guide that's trying to keep things simple. But I must admit I am a little worried as I've had a few too many experiences with people who believed that touching a moved-from object was almost always undefined behavior. Lots of bad "use-after-move is analogous to use-after-free" mental models.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-7xErcobBM%3DO%2BMf6g8VO4wLfWryH268DxkGBssctMFjFQ%40mail.gmail.com.

[Roland McGrath]

Often `x = std::move(y);` can be replaced with something like `x = std::exchange(y, {});`, which should compile down to essentially the same thing and is more unambiguously safe.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAAE6ATVZ2qn7yd-wsrj_tjCH7nWrpC_ju%2BrBCGTTSpwcgHgew%40mail.gmail.com.

[Daniel Cheng]

1. It's non-trivial to determine if a moved-from object is in a valid state or not. In //base (and Chrome), we should try to be clear about it, e.g.

  - moved-from smart pointers will be null

  - moved-from callbacks will be null

  - use-after-move is altogether disallowed for some other types (e.g. raw_ref<T>)

But it can be far less obvious for things in the STL

- is a moved-from std::string empty? (no, not necessarily)

- is a moved-from std::vector<T> empty? (usually but not always)

2. "They tend to gloss over the fact that objects often have methods that are perfectly safe to call after a move (especially stuff like a "reset" or "clear" that unconditionally puts the object into a known state) [...]  I must admit I am a little worried as I've had a few too many experiences with people who believed that touching a moved-from object was almost always undefined behavior."

- Our guidance specifically states that it's fine to assign to it.

- The original text was written before we had clang-tidy's use-after-move warnings. Methods like Reset() and clear() should be annotated with REINITIALIZES_AFTER_MOVE to explicitly allow their use to put something into a known state.

- I would support adding a bit more nuance but I would certainly not encourage writing code that depends on the moved-from state of a standard library object if it is not explicitly specified.

Daniel

[Peter Boström]

I would also like to chime in that it's a lot easier to keep a rule like "use-after-move is bad" in your head than all of the exceptions that exist to it.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKrT7Fmi5kBXgoCKsOVoEiJNbvA_EVW%3D%2B3Rb0nEzhf2_vw%40mail.gmail.com.

[K. Moon]

I think it's fine to author code assuming "use-after-move is bad (because it can be confusing)," but if it comes up in code review and the author can show that a particular move is safe and a reasonable way to write the code, I wouldn't push back (other than to maybe suggest a comment). I think this is a, "I don't like this as a reviewer," issue, rather than a, "Everyone agrees this is bad," issue, and generally follow the advice to defer to the author's judgment unless there's a clear problem.

[John Admanski]

I don't think the rules need to be loosened up much, other than generalizing the "you can assign to them" to other well defined REINITIALIZES_AFTER_MOVE stuff.

I do think it would be a lot better if the section was written in more of a "this is complicated and difficult to get right, so use these simpler rules" style. Similar to the way a lot of these types of restrictions are written up in style guide rules.

On Mon, Oct 24, 2022 at 10:14 AM Peter Boström <pb...@chromium.org> wrote:

[K. Moon]

FWIW, the first time I came across this article was when it was referenced in this thread. It comes across to me as more of an explanatory tutorial that was written up when C++11 became available in Chromium, rather than something meant to be a style rule; otherwise, I would expect this content to be in //styleguide/c++/, or at least linked from there. (As far as I can tell, it's not.)

[dan...@chromium.org]

That was the intent of it, yes, to introduce rvalues to everyone for the first time.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-7rxpnJ9XNpYbt5kH9oEpE1A6Yxgk_aCF%3DT4sG_MdNiXw%40mail.gmail.com.

[Gabriel Charette]

The one crux of leaning into "this type is always reset after a move" is that not every dev understands that std::move on its own isn't a move, without it being consumed, and the semantics of "being consumed" are tricky (i.e. passing to another function isn't sufficient, that function could be accepting a const& and not consume the move).

If we do decide to lean into "this type is always reset after a move", I had proposed a helper (base::ResetOnMove<T>) to turn a POD <T> into a T that's auto-reset on move (e.g. an int set back to 0).

This would be particularly useful in allowing more move constructor/operator to be `= default;`. Currently, if a single member requires custom move logic, the move constructor/operator needs to explicitly declare move semantics for *each* member (error prone and not future proof).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAHtyhaT%3DZEBODVq_h6AUeUCuGM54VvaMN3Zyd_bMwRipqkmEWQ%40mail.gmail.com.

[Christian Biesinger]

I sent https://chromium-review.googlesource.com/c/chromium/src/+/3991393 for CallbackBase::Reset in particular.

But it seems like if nothing else, https://www.chromium.org/rvalue-references/#5-you-must-not-use-a-variable-after-calling-stdmove-on-it should be updated to also allow calling reset methods if annotated with REINITIALIZES_AFTER_MOVE?

Christian

[dan...@chromium.org]

On Fri, Oct 28, 2022 at 5:42 PM Christian Biesinger <cbies...@chromium.org> wrote:

I sent https://chromium-review.googlesource.com/c/chromium/src/+/3991393 for CallbackBase::Reset in particular.

But it seems like if nothing else, https://www.chromium.org/rvalue-references/#5-you-must-not-use-a-variable-after-calling-stdmove-on-it should be updated to also allow calling reset methods if annotated with REINITIALIZES_AFTER_MOVE?

It sounds good, and I think anyone can edit the page.

[Christian Biesinger]

On Fri, Oct 28, 2022 at 5:52 PM <dan...@chromium.org> wrote:

On Fri, Oct 28, 2022 at 5:42 PM Christian Biesinger <cbies...@chromium.org> wrote:

I sent https://chromium-review.googlesource.com/c/chromium/src/+/3991393 for CallbackBase::Reset in particular.

But it seems like if nothing else, https://www.chromium.org/rvalue-references/#5-you-must-not-use-a-variable-after-calling-stdmove-on-it should be updated to also allow calling reset methods if annotated with REINITIALIZES_AFTER_MOVE?

It sounds good, and I think anyone can edit the page.

It looks like the website is in a git repository now, I sent you https://chromium-review.googlesource.com/c/website/+/3991726

Christian