Incorporate LWG feedback

[gnzlbg]

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

• 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. CHANGE: added a "Note to LWG" about why LEWG agreed that this should be a pre-condition violation.

• All Requires that cannot be checked at compile-time should be Expects

• UNCLEAR: LWG recommended moving this into the <vector> header. LEWG's intent was to make the static_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 for static_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 and Requires ?

Section 5.1

• 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.

• The note about incomplete types is redundant and should be removed.

• difference_type has to use ptrdiff_t instead of make_signed<size_t>

• 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 conditional noexcept on these are removed.

• INVALID: There are no deduction guides. The reviewers realized that the capacity cannot be deduced.

• Instead of using: DefaultInserted, CopyInsertable, EmplaceConstructible, ... which require an allocator, these proposal should directly use, e.g., std::is_..._constructible instead.

  • UNCLEAR: DefaultInserted can become value initialized. RESOLVED: DefaultInserted is no longer mentioned.

• Add static_vector to the tables of library and freestanding headers.

• Add static_vector to the exceptions in the container requirement tables for array.

Section 5.2 Constructors

• 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:

  The move semantics of static_vector<T, Capacity> are equal to those of std::array<T, Size>

  static_vector a(10);
  static_vector b(std::move(a));

  the elements of a have been moved element-wise into b, the elements of a are left in an initialized
  but unspecified state (have been moved from state), the size of a is not altered, and a.size() == b.size().

  Note: this behavior differs from std::vector<T, Allocator>, in particular for the similar case in which std::allocator_traits<Allocator>::propagate_on_container_move_assignment is false. In this situation the state of std::vector is initialized but unspecified.

• The iterator constructor complexity should read: "Complexity: Linear in distance(first, last)."

• Iterator constructor; change: "value_type shall be EmplaceConstructible into *this from *first and distance(first, last) <= capacity()" to:

  • Mandates: std::is_constructible<value_type, decltype(*first)>
  • Expects: distance(first, last) <= capacity()

5.3 Destructor

• Effects has to state that the elements are destroyed.

• Change the Requires clause to: "This destructor is trivial if the destructor of value_type is trivial.".

5.4 Size and capacity

• Get rid of the semicolons at the end of each line (EDIT: this breaks markdown syntax highlighting).

• Replace "Effects: equivalent to return N;." to "Returns: N."

• Fix c++-artefact near the resize overloads.

• Each of the resize overloads should have its own section.

• The remarks of the resize overloads are wrong:

  • UNCLEAR: Not all constexpr types are trivially copyable. What is a "constexpr type" ? What would be the right constraint?
  • Removed the remark. The overloads are now constexpr, nothing is said about when this is the case.

• Resize overloads: the is_default_constructible_v is only needed for the resize(size_type sz) overload

5.6 Modifies

• 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()."

• The modifiers need to be split into different sections. For insert we need to care about exception behavior, but for push_back we don't. So at least two different sections for insert and push_back.

• UNCLEAR: Throws doesn't apply here

• 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.

• 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 incorrect size()). So this needs to somehow call out that such an implementation is illegal.

• "anassignment" should be "an assignment"

• 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 ...". ?

• Split sections of pop_back() and erase() .

• UNCLEAR "the complexity [of pop back and/or erase] doesn't make any sense"

• UNCLEAR "the note doesn't make any sense"

  • UNCLEAR: The Note explains why these methods can't be 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 ?

• Swap member function should take a reference.

• 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 and max(size(), other.size()) - min(size(), other.size()) move constructions." or similar.

• noexcept of member swap is wrong as there might be different number of elements in the containers. It should probably be changed from noexcept(is_nothrow_swappable_v<value_type>) to noexcept(is_nothrow_swappable_v<value_type> && is_nothrow_move_constructible_v<value_type>).

5.7 specialized algorithms

• Replace Remarks with Constraints

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.

[mclow]

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.

[mclow]

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.

[CaseyCarter]

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.

[CaseyCarter]

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.

[CaseyCarter]

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.

[mclow]

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.

[CaseyCarter]

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.

[CaseyCarter]

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.

[CaseyCarter]

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.

[CaseyCarter]

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.

[CaseyCarter]

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 ;)

[gnzlbg]

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.

[mclow]

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.

[mclow]

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.

[gnzlbg]

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).

[CaseyCarter]

@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?

[gnzlbg]

@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:

• whether this container should live in <vector> instead of being stand alone,
• whether this container should throw exceptions when operations exceed its capacity instead of invoking undefined behavior,

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).