-
Notifications
You must be signed in to change notification settings - Fork 769
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
Conversation
0762097
to
a95bd79
Compare
a95bd79
to
ba6ee30
Compare
There was a problem hiding this 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.)
source/containers.tex
Outdated
%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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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}. |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
9d0a05e
to
0aac6fe
Compare
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.
0aac6fe
to
37a6c0d
Compare
Editor notes:
Fixes #7090
Fixes cplusplus/papers#114
Notes/questions on paper:
[containers.sequences.general] is now [sequences.general],[container.requirements.sequence.reqmts] is now [sequence.reqmts], etc..