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

[LWG 7 2024-06] P0843R14 inplace_vector #7115

Merged
merged 2 commits into from
Jul 16, 2024
Merged

[LWG 7 2024-06] P0843R14 inplace_vector #7115

merged 2 commits into from
Jul 16, 2024

Conversation

burblebee
Copy link
Contributor

@burblebee burblebee commented Jul 2, 2024

Editor notes:

  • Unable to apply change to Complexity for "a.swap(b)" in section [container.reqmts]; only such \itemdecl is in [container.alloc.reqmts] with different wording. UPDATE: found and fixed
  • Unable to apply change to note in "sequence.reqmts.2" - wording does not appear to exist. UPDATE: wording was removed
  • Sections named [containers.sequences.inplace.vector.*] renamed to [inplace.vector.*].
  • [inplace.vector.modifiers] Add commas for clarity.
  • [inplace.vector] Minor editorial fixes (e.g. add missing "the").

Fixes #7090
Fixes cplusplus/papers#114

Notes/questions on paper:

  • Paper wants [inplace_vector] after [vector], but wouldn't it make more sense to put it after [vector.bool]? UPDATE: We probably do want [inplace_vector] after [vector.bool] - should add as a separate commit before merging (for me to do this now would complicate the review)
  • Sections have been renamed from what's in the paper, e.g.
    [containers.sequences.general] is now [sequences.general],[container.requirements.sequence.reqmts] is now [sequence.reqmts], etc..
  • See added FIXMEs.

@burblebee burblebee force-pushed the motions-2024-06-lwg-7 branch from 0762097 to a95bd79 Compare July 3, 2024 00:36
@burblebee burblebee marked this pull request as ready for review July 3, 2024 00:40
@burblebee burblebee marked this pull request as draft July 3, 2024 05:57
@burblebee burblebee force-pushed the motions-2024-06-lwg-7 branch from a95bd79 to ba6ee30 Compare July 3, 2024 06:06
@burblebee burblebee marked this pull request as ready for review July 3, 2024 06:16
@burblebee burblebee requested a review from frederick-vs-ja July 9, 2024 22:41
Copy link
Contributor

@Quuxplusone Quuxplusone left a comment

Choose a reason for hiding this comment

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

Editor notes:

  • Unable to apply change to Complexity for "a.swap(b)" in section [container.reqmts]; only such \itemdecl is in [container.alloc.reqmts] with different wording.

The wording you're looking for was changed to t.swap(s) in 47fb595. That diff is still important and should be applied.

  • Unable to apply change to note in "sequence.reqmts.2" - wording does not appear to exist.

That wording was removed in #6571, commit 2b5fc29. Therefore, not-applying that diff seems appropriate.

  • Paper wants [inplace_vector] after [vector], but wouldn't it make more sense to put it after [vector.bool]?

Yes, I'm sure that was the intent. (Recall that the author assumed section headings were hierarchical, so that [vector.bool] would have been treated as part of [vector] from P0843's point of view. "After all the <vector> stuff" is clearly the intent.)

Comment on lines 9276 to 9532
%FIXME: Should this instead say something like:
%FIXME: "For any \tcode{N} such that \tcode{N > 0} is \tcode{true}, ..."?
%FIXME: Same for similar expressions added in P0843R14?
For any \tcode{N > 0},
if \tcode{is_trivial_v<T>} is \tcode{false}, then
no \tcode{inplace_vector<T, N>} member functions
are usable in constant expressions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
%FIXME: Should this instead say something like:
%FIXME: "For any \tcode{N} such that \tcode{N > 0} is \tcode{true}, ..."?
%FIXME: Same for similar expressions added in P0843R14?
For any \tcode{N > 0},
if \tcode{is_trivial_v<T>} is \tcode{false}, then
no \tcode{inplace_vector<T, N>} member functions
are usable in constant expressions.
If \tcode{is_trivial_v<T>} is \tcode{false}, then
no member functions of \tcode{inplace_vector}
are usable in constant expressions.

Here and above, there's no need to say "for all N" nor "for all T", because we're already within the context of inplace_vector, i.e., the injected-class-name of some particular specialization of inplace_vector, with a particular T and N.

Note to anyone who cares: This used to say something to the effect of "no non-static member functions of", but LWG discussion in St Louis seems to have come down on the side of "who cares if e.g. capacity is constexpr-friendly, when size itself isn't," and so "no member functions [period]" is indeed what was polled and approved.

Copy link
Contributor

@frederick-vs-ja frederick-vs-ja Jul 10, 2024

Choose a reason for hiding this comment

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

I guess it's more appropriate to say

no member functions of inplace_vector are guaranteed to be usable in constant expressions

or

it is unspecified whether any member function of inplace_vector is usable in constant expressions

But this seemingly needs an LWG issue, and the sentence will be dropped if P3074 lands.

Copy link
Contributor

Choose a reason for hiding this comment

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

By my recollection, LEWG explicitly discussed the fact that library vendors have the freedom to add noexcept but not to add constexpr, and we were explciitly deciding to forbid library vendors from making any of these functions constexpr-friendly in those cases.

That was certainly given as a rationale for why iterator is unconditionally constexpr-friendly: at least some of us were under the impression that if we had said "Under such-and-such conditions iterator does not meet the constexpr iterator requirements," then we would be forcing library vendors to invent an iterator type that was "basically T* but constexpr-unfriendly," just for that purpose.

However, if (my recollection of) our impression was wrong — if library vendors actually do have the freedom to make inplace_vector "more constexpr than strictly necessary" — well, IMHO that wouldn't be a bad outcome at all. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've left this as is - this wording (e.g. the use of inplace_vector<T, N>) is used throughout this section; to change it only here would be inconsistent, while changing it elsewhere requires additional wording.

Copy link
Member

Choose a reason for hiding this comment

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

Here and above, there's no need to say "for all N" nor "for all T",

But it doesn't say for all N, it says for N > 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is being moot as the restriction itself is being removed by P3074.

\pnum
\throws
Nothing unless an exception is thrown by
the assignment operator or move assignment operator of \tcode{T}.
Copy link
Contributor

Choose a reason for hiding this comment

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

"The move assignment operator," said Ford Prefect quietly, "is also an assignment operator."

If this is trying to hint that we might use an assignment operator other than the move-assignment operator (e.g. if the only operator= available is void operator=(std::any), like this) — well, that's true, but I still think there's no sense in mentioning a subset of assignment operators (the move assignment operators) here.

Suggested change
the assignment operator or move assignment operator of \tcode{T}.
the assignment operator of \tcode{T}.

Cf. the Complexity element, which gets it right, in my book.

Copy link
Contributor Author

@burblebee burblebee Jul 10, 2024

Choose a reason for hiding this comment

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

this is a normative change, please forward to LWG.

Copy link
Member

Choose a reason for hiding this comment

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

It should either be "the copy assignment operator or move assignment operator" or "an assignment operator", but the approved wording and the suggested change are both bad.

Copy link
Contributor

Choose a reason for hiding this comment

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

See also #3208. It was very unfortunate that the bad wording got copied.

I think "the copy assignment operator or move assignment operator" is still defective, because T can be copy/move-assigned by an assignment operator which is a template when the real copy/move assignment operator is not selected.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jwakely Could we turn this, and #3208, into an LWG issue?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@burblebee burblebee requested a review from Quuxplusone July 10, 2024 12:59
@tkoeppe tkoeppe force-pushed the motions-2024-06-lwg-7 branch 3 times, most recently from 9d0a05e to 0aac6fe Compare July 13, 2024 00:07
@tkoeppe tkoeppe requested review from jwakely and timsong-cpp July 13, 2024 00:59
burblebee and others added 2 commits July 16, 2024 09:37
Editorial notes:
* The wording in the paper is based on an old draft, and several
  intervening changes had to be incorporated into the application.
* Specifically, Note 1 in [sequence.reqmts] no longer exists (and
  thus does not get updated).
* Stable labels named [containers.sequences.inplace.vector.*] renamed to [inplace.vector.*].
* [inplace.vector.modifiers] Add comma for clarity.
* [inplace.vector] Minor editorial fixes (e.g. add missing "the").
* [inplace.vector.overview] Fix punctuation in bulleted sub-list to be consistent.
@tkoeppe tkoeppe force-pushed the motions-2024-06-lwg-7 branch from 0aac6fe to 37a6c0d Compare July 16, 2024 08:48
@tkoeppe tkoeppe merged commit 773c18a into main Jul 16, 2024
4 checks passed
@jensmaurer jensmaurer deleted the motions-2024-06-lwg-7 branch November 24, 2024 19:34
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.

[2024-06 LWG Motion 7] P0843R14 inplace_vector P0843 R14 inplace_vector
6 participants