Open gnzlbg opened 5 years ago
UNCLEAR: Exceeding the container capacity (e.g. on push_back): in revision 2 this is a pre-condition violation. Is LWG recommending that this should std::abort instead? The notes are unclear.
What I remember saying is that IMO this should throw an exception. If you build with exceptions turned off, then both libc++ and libstdc++ will call abort
instead of throwing an exception.
NON ACTIONABLE: DefaultInserted, CopyInsertable, EmplaceConstructible require an allocator, but static_vector does not have an allocator. Unclear what actionable change can be pursued in the static_vector proposal. Should this proposal modify those concepts ?
No - don't modify those concepts.
What this means is that you'll have to scavenge wording from somewhere else, instead of using these pre-existing concepts. You can either copy the wording from those concepts, removing the allocator bits, but I would look at array
first - since it is also a non-allocator aware container.
UNCLEAR: Exceeding the container capacity (e.g. on push_back): in revision 2 this is a pre-condition violation. Is LWG recommending that this should std::abort instead? The notes are unclear.
What I remember saying is that IMO this should throw an exception. If you build with exceptions turned off, then both libc++ and libstdc++ will call
abort
instead of throwing an exception.
This design decision was discussed at length and approved in LEWG. One of the primary use cases for this container is embedded/freestanding: people don't want exceptions in the interface. UB allows throwing an exception, so you are free to do so in your implementation.
NON ACTIONABLE: DefaultInserted, CopyInsertable, EmplaceConstructible require an allocator, but static_vector does not have an allocator. Unclear what actionable change can be pursued in the static_vector proposal. Should this proposal modify those concepts ?
No - don't modify those concepts.
What this means is that you'll have to scavenge wording from somewhere else, instead of using these pre-existing concepts. You can either copy the wording from those concepts, removing the allocator bits, but I would look at
array
first - since it is also a non-allocator aware container.
The allocation concepts do not require an allocator; http://eel.is/c++draft/container.requirements.general#15: "If X
is not allocator-aware, the terms below are defined as if A
were allocator<T>
— no allocator object needs to be created and user specializations of allocator<T>
are not instantiated". That said, I'd be happier to replace these requirements with Concepts now that we have them.
LWG recommended moving this into the
<vector>
header. The<static_vector>
header is free-standing. Will the<vector>
header be made free-standing?
It may some day, but almost certainly not for C++20. I don't think the freestanding nature was clear to LWG during their review. We need to add an entry to the table in [headers] for <static_vector
>, and - most importantly - a row to the table that lists the freestanding headers in [compliance]. The latter would have avoided confusion about the freestanding nature of static_vector
.
I'm not enthusiastic about "we need a new header because it's freestanding". Not to mention the fact that "what is freestanding" is decidedly unclear - and changing.
I'm not enthusiastic about "we need a new header because it's freestanding".
Would you prefer that it goes into one of the pre-existing freestanding headers? The choices are:
<cstddef>
<cfloat>
<limits>
<climits>
<version>
<cstdint>
<cstdlib>
<new>
<type_info>
<exception>
<initializer_list>
<cstdarg>
<type_traits>
<bit>
<atomic>
None of these seem appropriate to me.
EDIT: I'm being facetious, but the fact remains that we can either put this in its own header or a pre-existing header. <vector>
is almost certainly not provided by freestanding implementations since it requires allocation; <array>
would be at least borderline.
Remove conditional
noexcept
on copy-constructors and copy-assignment?
I don't recall specific discussion of this noexcept
in LEWG. Our general policy is to only use conditional noexcept
on moves and swaps. Going against the general policy requires discussion and approval of the rationale in LEWG and (ideally) a recorded poll of LEWG's approval in the paper so LWG knows that discussion and approval occurred.
I'd suggest simply striking these. I find nothing else questionable from a quick audit of the remaining appearances of noexcept
in the synopsis.
There are no deduction guides by design. Should this be mentioned in a note?
No - the reviewers appear to have realized that the capacity can't be deduced, so an iterator-pair deduction guide wouldn't be implementable. I don't think there's any action needed here.
Spell out move constructor - reviewers mentioned that it is unclear for example if it needs to preserve the elements.
Table 64 in [containers.requirements.general] specifies the semantics of container move constructors - I can only assume the reviewers were confused.
We do need to add static_vector
to the subsequent paragraph which states that some operations are linear for array
.
EDIT: We should steal [array.cons]/1 - with the exception of the aggregate requirement - to help clarify.
The iterator constructor complexity should be linear in the distance.
I interpret this as a suggestion that the complexity should literally read "Complexity: Linear in distance(first, last)
."
EDIT: We certainly don't want to duplicate vector
's defective: "Makes only N
calls to the copy constructor of T
" wording ;)
So I hope I have addressed all of LWGs comments, and I hope I have properly incorporated all of @CaseyCarter feedback in the @master branch. I have ticked almost all of the boxes on the LWG feedback, but I'm still not 100% sure how to interpret all of it.
It would be really really helpful if, in particular, those who participated in LWGs review could revisit their feedback and see if the changes proposed addresses it or not. I don't really have a way to reach everyone involved although I've tried to CC everyone I could find on github. I understand that nobody has time for this, but I'd like to at least know if I should re-submit it back to LEWG as proposed by the review, or to LWG.
What I remember saying is that IMO this should throw an exception. If you build with exceptions turned off, then both libc++ and libstdc++ will call
abort
instead of throwing an exception.This design decision was discussed at length and approved in LEWG. One of the primary use cases for this container is embedded/freestanding: people don't want exceptions in the interface. UB allows throwing an exception, so you are free to do so in your implementation.
We have an entire study group dedicated to reduce the amount of UB in the language and the library. AND we have a bunch of people deliberately trying to put more in.
This seems wrong to me.
What I remember saying is that IMO this should throw an exception. If you build with exceptions turned off, then both libc++ and libstdc++ will call
abort
instead of throwing an exception.This design decision was discussed at length and approved in LEWG. One of the primary use cases for this container is embedded/freestanding: people don't want exceptions in the interface. UB allows throwing an exception, so you are free to do so in your implementation.
I've reviewed the current wording for "freestanding", (in [compliance]) and nowhere in there does it talk about not having exceptions.
As for which headers are part of a freestanding implementation, the wording is:
A freestanding implementation has an implementation-defined set of headers. This set shall include at least the headers shown in Table 21.
This seems wrong to me.
@mclow Everybody agrees that this is sub-optimal but the current proposal is the result of the best ideas people had. Pretty much every other approach considered had significant downsides. Do you have a better idea?
What do you think about making this unspecified, but specifying that it does not result in undefined behavior?
That would still allow us to change this to use contracts in the future. In the meantime implementations are still allowed to abort, throw something, loop forever, ... as long as undefined behavior is avoided.
Alternatively, we could make this implementation defined, and provide some options, but changing this to contracts wouldn't be a backwards compatible change (we could add contracts as an alternative in the future, and recommend this, but removing the other options would be a breaking change).
@mclow Would you mind sending a brief summary of your design concerns to Titus, and requesting that LEWG review the pertinent decisions - ideally on the reflector to save us an extra meeting cycle?
@mclow @CaseyCarter
Would you mind sending a brief summary of your design concerns to Titus, and requesting that LEWG review the pertinent decisions - ideally on the reflector to save us an extra meeting cycle?
Did this happen? If so, is there a link to the discussion in the reflector?
If not, given that the deadline for papers is next week, I will re-submit the paper with these fixes to LEWG so that they can re-evaluate the two major issues about the design that @mclow raised:
<vector>
instead of being stand alone,I'll try my best to gather all the thoughts expressed about these issues so that LEWG can have a fruitful discussion. It would be helpful if @mclow could send me a bare draft of their concerns, or if, once I commit the concerns to github, @mclow could review that they reflect their opinions accurately (I'll ping them in a PR for that).
LWG gave the following feedback on the second revision. I'll open new issues to tackle controversial change. I have tried to call out UNCLEAR and NON ACTIONABLE requests. A ticked checkbox indicates that the issue has been fixed on the master branch:
General comments
[x] UNCLEAR: Exceeding the container capacity (e.g. on
push_back
): in revision 2 this is a pre-condition violation. Is LWG recommending that this shouldstd::abort
instead? The notes are unclear. CHANGE: added a "Note to LWG" about why LEWG agreed that this should be a pre-condition violation.[x] All
Requires
that cannot be checked at compile-time should beExpects
[x] UNCLEAR: LWG recommended moving this into the
<vector>
header. LEWG's intent was to make thestatic_vector
component freestanding. CHANGE: add changes required to make<static_vector>
header freestanding, and add note to LWG indicating that currently, none of the existing freestanding headers is a good fit forstatic_vector
.[ ] UNCLEAR: "the design should be fixed" it is unclear to which part of the design this final comment refers to. Also it is unclear whether the next iteration of this proposal should be sent to.
[ ] UNCLEAR: what is the difference between
Mandates
andRequires
?Section 5.1
[x] 5.1.ii: states "meets all requirements of containers", and then gives exceptions. Instead, it should contain a sentence per container category (container, reversible container, sequence container, and contiguous container), where each sentence list the limitations for each category.
[x] The note about incomplete types is redundant and should be removed.
[x]
difference_type
has to useptrdiff_t
instead ofmake_signed<size_t>
[x] Remove conditional
noexcept()
on copy-constructors and copy-assignment. Our general policy is to only use conditional noexcept on moves and swaps. Going against the general policy requires discussion and approval of the rationale in LEWG and (ideally) a recorded poll of LEWG's approval in the paper so LWG knows that discussion and approval occurred. This was not discussed in LEWG, so the conditionalnoexcept
on these are removed.[x] INVALID: There are no deduction guides. The reviewers realized that the capacity cannot be deduced.
[x] Instead of using:
DefaultInserted
,CopyInsertable
,EmplaceConstructible
, ... which require an allocator, these proposal should directly use, e.g.,std::is_..._constructible
instead.DefaultInserted
can become value initialized. RESOLVED:DefaultInserted
is no longer mentioned.[x] Add
static_vector
to the tables of library and freestanding headers.[x] Add
static_vector
to the exceptions in the container requirement tables forarray
.Section 5.2 Constructors
[x] Spell out move constructor - reviewers mentioned that it is unclear for example if it needs to preserve the elements.
From the proposal revision 2, Section 4.5 Move Semantics:
[x] The iterator constructor complexity should read: "Complexity: Linear in
distance(first, last)
."[x] Iterator constructor; change: "
value_type
shall beEmplaceConstructible
into*this
from*first
anddistance(first, last) <= capacity()
" to:std::is_constructible<value_type, decltype(*first)>
distance(first, last) <= capacity()
5.3 Destructor
[x] Effects has to state that the elements are destroyed.
[x] Change the Requires clause to: "This destructor is trivial if the destructor of
value_type
is trivial.".5.4 Size and capacity
[x] Get rid of the semicolons at the end of each line (EDIT: this breaks markdown syntax highlighting).
[x] Replace "Effects: equivalent to return N;." to "Returns: N."
[x] Fix
c++
-artefact near theresize
overloads.[x] Each of the
resize
overloads should have its own section.[x] The remarks of the
resize
overloads are wrong:constexpr
types are trivially copyable. What is a "constexpr
type" ? What would be the right constraint?constexpr
, nothing is said about when this is the case.[x] Resize overloads: the
is_default_constructible_v
is only needed for theresize(size_type sz)
overload5.6 Modifies
[x] Replace "Requires: The number of elements to be inserted shall be at most C - size()." with "Expects: The number of elements to be inserted shall be at most
capacity() - size()
."[x] The modifiers need to be split into different sections. For
insert
we need to care about exception behavior, but forpush_back
we don't. So at least two different sections forinsert
andpush_back
.[x] UNCLEAR: Throws doesn't apply here
[x] UNCLEAR:
push_back()
or append at the end should be strong exception safe. Which part of the specification makes this not clear? The Remarks specifically states that this is the case.[x] UNCLEAR: The text seems to be copied from something allocating which doesn't apply here. Which part of the text does not apply here? For example, if the modifiers are implemented such that the
size()
of the container is modified before calling, e.g, a move/copy constructor/assignment to insert something, and it throws, then there will be effects (the vector will have an incorrectsize()
). So this needs to somehow call out that such an implementation is illegal.[x] "anassignment" should be "an assignment"
[x] UNCLEAR: The Complexity is wrong. Unclear what is wrong about the complexity. Should it say "number of elements inserted plus the distance from the insertion point to the end of the static_vector." instead of "linear in the number of ...". ?
[x] Split sections of
pop_back()
anderase()
.[x] UNCLEAR "the complexity [of pop back and/or erase] doesn't make any sense"
[x] UNCLEAR "the note doesn't make any sense"
noexcept
. This is correct, that is what the note does. It is a note intended for the reader of the proposal, not for standard wording. Should I remove all the notes that are intended for the reader in the next version of the proposal ?[x] Swap member function should take a reference.
[x] Complexity of swap is incorrect. It was suggested to change it to "Complexity: number of the elements exchanged" but also suggested that "not all elements are exchanged". Therefore, I propose to change it to "Complexity:
min(size(), other.size())
swaps andmax(size(), other.size()) - min(size(), other.size())
move constructions." or similar.[x]
noexcept
of member swap is wrong as there might be different number of elements in the containers. It should probably be changed fromnoexcept(is_nothrow_swappable_v<value_type>)
tonoexcept(is_nothrow_swappable_v<value_type> && is_nothrow_move_constructible_v<value_type>)
.5.7 specialized algorithms
cc @mclow @BRevzin @dietmarkuehl @jwakely @CaseyCarter
Note to self: once I finish incorporating all the feedback in the proposal it will be time to update the implementation to match the proposal.