Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[range.nonprop.cache] Clarify that emplace-deref does not require materialization of the result of *i before the initialization #4732

Merged
merged 2 commits into from Jul 21, 2021

Conversation

timsong-cpp
Copy link
Contributor

Based on empirical evidence, the existing wording is too subtle, so this adds a note to make the implication more obvious.

@timsong-cpp timsong-cpp changed the title [range.nonprop.cache] Clarify that emplace-deref does not require materialization of the result of *i [range.nonprop.cache] Clarify that emplace-deref does not require materialization of the result of *i before the initialization Jul 3, 2021
@tkoeppe
Copy link
Contributor

tkoeppe commented Jul 5, 2021

Out of curiosity, could you give an example for which this is_constructible is false?

@JohelEGP
Copy link
Contributor

JohelEGP commented Jul 5, 2021

#include <new>
#include <type_traits>

struct S {
  S() = default;
  S(const S&) = delete;
  S(S&&) = delete;
};

S deref() { return S{}; }
S s{};

void emplace_deref() {
  s.~S();
  new (&s) S(deref());
}

static_assert(!std::is_constructible_v<S, S>);

https://godbolt.org/z/GbsxT8P34

@tkoeppe
Copy link
Contributor

tkoeppe commented Jul 5, 2021

Thanks! Should we maybe augment the note to say "... is_constructible can be false, but the above declaration of t can still be valid."? Or "... the above declaration can be valid even if is_constructible is false"?

@jensmaurer
Copy link
Member

jensmaurer commented Jul 5, 2021

Or maybe: "If *i is a prvalue, there is no requirement that it is copyable (dcl.init.general)."

@tkoeppe
Copy link
Contributor

tkoeppe commented Jul 5, 2021

Maybe even, "if *i is a prvalue, i can be passed to emplace_deref without a requirement that *i is copyable (dcl.init.general)"?

@jensmaurer
Copy link
Member

Repeating "emplace_deref doesn't seem to add anything.

@jensmaurer jensmaurer added the decision-required A decision of the editorial group (or the Project Editor) is required. label Jul 7, 2021
@jensmaurer
Copy link
Member

Editorial meeting 2021-07-16: Go with Jens' suggestion.

@timsong-cpp , please update your pull request accordingly.

@jensmaurer jensmaurer added changes requested Changes to the wording or approach have been requested and not yet applied. and removed decision-required A decision of the editorial group (or the Project Editor) is required. labels Jul 16, 2021
@timsong-cpp
Copy link
Contributor Author

I used "movable" instead since emplace(*i) would be a move rather than a copy.

Copy link
Member

@jensmaurer jensmaurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good.

@jensmaurer jensmaurer removed the changes requested Changes to the wording or approach have been requested and not yet applied. label Jul 20, 2021
@jensmaurer
Copy link
Member

jensmaurer commented Jul 20, 2021

@tkoeppe , this seems ready to merge now.
Make sure to squash the commits.

@tkoeppe tkoeppe merged commit 1305740 into cplusplus:main Jul 21, 2021
@timsong-cpp
Copy link
Contributor Author

@tkoeppe The commit message on 1305740 is incorrect:

The normatively expressed requirement in terms of an invented initialization of a variable was prone to confusion, since such a declaration would indeed require *i to be copyable/movable, whereas the actual use of emplace-deref does not
cause any intermediate materializations of prvalues.

That invented initialization does not require *i to be copyable or movable in this case. That's exactly why it is specified this way and not with is_constructible_v or constructible_from, and the whole reason for the note's existence (and why it is a note rather than normative wording).

In the future, I would appreciate an opportunity to comment on this kind of commit messages that are going to be attributed to me before they end up in main, instead of discovering them afterwards.

@jensmaurer
Copy link
Member

jensmaurer commented Jul 26, 2021

@tkoeppe, this looks like the time for a force-push of master.

@tkoeppe
Copy link
Contributor

tkoeppe commented Jul 26, 2021

@timsong-cpp Sorry about that! You're right -- could you please provide a new message (that explains the clarified subtlety), and I'll update that?

@timsong-cpp
Copy link
Contributor Author

How about:

A new note explains that emplace-deref requires implementations to avoid
materialization of the result of *i before the initialization. This is implied by the
normatively expressed requirement in terms of an invented initialization of a
variable, but is easily overlooked.

tkoeppe pushed a commit that referenced this pull request Jul 28, 2021
A new note explains that `emplace-deref` requires implementations to
avoid materialization of the result of `*i` before the initialization.
This is implied by the normatively expressed requirement in terms of
an invented initialization of a variable, but is easily overlooked.
@tkoeppe
Copy link
Contributor

tkoeppe commented Jul 28, 2021

Thank you, I updated that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants