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

[array.zero] Fix triple comparison and improve wording consistency #6807

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Eisenwave
Copy link
Contributor

@Eisenwave Eisenwave commented Feb 15, 2024

[array.zero] p1

This paragraph is not normative and basically pointless. The existence of a section called "Zero-sized arrays" in itself implies that there is some special support for zero-sized arrays. It's also entirely unclear what it means to "provide support"; no concrete demand is being made here. Therefore, this paragraph should be deleted.

[array.zero] p2

This paragraph contains the broken expression begin() == end() == unique value. Such a triple comparison would work in mathematical notation, but in C++, this implies nothing for begin() == end(), only that "unique value" is being equality-compared to an expression of type bool.

In general, the convention is to write "x == y is true" or "x equals y" when something normative is expressed, since a C++ expression does not have any such implication, even if it is of type bool.

Furthermore, it is entirely unclear what "unique value" is in this context. Does it mean that every invocation of begin() and end() produces a unique value, and begin() and end() compare equal? Does it mean that the result of the expression of begin() == end() is a unique value for each comparison?

I'm not even sure if anything normative is being expressed here, and the intent is clearly just to say that begin() equals end() for empty arrays. Even if it said "begin() returns a unique value", what would that actually mean? A value "being unique" does not have any obvious standard meaning, so it could be seen as fluff.

[array.zero] p3

This paragraph is the least problematic, but uses language inconsistent with p2. It would be more consistent if all paragraphs began with the same prefix.

[array.zero] p4

This paragraph has a similar issue as p4. The intent is obviously not that swap() is always noexcept because otherwise, it would be noexcept in the synopsis. This paragraph is located in [array.zero] so presumably, it is only meant to apply to zero-length arrays. Let's prefix it and make this clear.

source/containers.tex Outdated Show resolved Hide resolved
Copy link
Member

@jwakely jwakely left a comment

Choose a reason for hiding this comment

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

This seems like a net improvement, and editorial, but has a lot of overlap with LWG 2157

In the case that \tcode{N == 0}, \tcode{begin() == end() ==} unique value.
The return value of \tcode{data()} is unspecified.
In the case that \tcode{N} equals \tcode{0},
the effect of calling \tcode{front()} or \tcode{back()} undefined.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be better addressed by putting a precondition on the front and back functions that N > 0. Does such a precondition fall out of the sequence container requirements already? If so, this should be a note, not a normative statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://eel.is/c++draft/sequence.reqmts#itemdecl:20 front() and back() dereference iterators.

To me, it seems like a better overall wording strategy is to make explicitly disallow this for empty arrays, like in LWG 2157.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intended not to mention operator[]?

Copy link
Contributor

Choose a reason for hiding this comment

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

My main point is that if this is already a precondition, the text here should be a Note, not (redundantly) normative. Do not make array<T, 0> seem any more special than it needs to be.

\tcode{array} shall provide support for the special case \tcode{N == 0}.
In the case that \tcode{N} equals \tcode{0},
\tcode{begin()} equals \tcode{end()} and
the return value of \tcode{data()} is unspecified.
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 a normative change, not editorial, as it gives up the requirement that different empty array objects return iterators with a different value. That point may be deemed moot as it is observed only by comparing pointers from different ranges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could observe a difference if the iterator was of pointer type and std::less compared iterators from two empty arrays. The intent of the current wording seems to be that the iterators from two separate arrays cannot be equivalent according to the total ordering of pointers.

Copy link
Contributor

Choose a reason for hiding this comment

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

The intent of the current wording seems to be that the iterators from two separate arrays cannot be equivalent according to the total ordering of pointers.

My reading is that this paragraph intended to guarantee that

  • an iterator obtained from array<T, 0> object and a value-initialized iterator never compare equal; and
  • two iterators obtained from two different array<T, 0> objects never compare equal.

And without such guarantees the comparison has (or can have, as decided by the library implementation) undefined behavior ([iterator.concept.forward]/2, [forward.iterators]/2).

However, it is not clear whether the requirement on unique value overrides the specification for forward iterators, and implementations now consistently return value-intialized iterators (libc++ was an exception and conformed to such guarantees, but is not now). I think we should open an LWG issue to drop "unique value".

@Eisenwave
Copy link
Contributor Author

This seems like a net improvement, and editorial, but has a lot of overlap with LWG 2157

Well thanks. I think it was editorial up to a point but removing the requirement for uniqueness of iterators is not.

LWG 2157 overhauls the whole section anyway, but I still believe there would be value in some very minor editorial changes. That triple comparison in p2 should be cleaned up for example. I'll scale this PR down.

Co-authored-by: Jonathan Wakely <github@kayari.org>
@jwakely
Copy link
Member

jwakely commented Feb 15, 2024

The uniqueness requirement for iterators is actually problematic for constexpr iterators. We don't have a pointer to a T that we can return, and we can't do something like reinterpret_cast<T*>(this) in constexpr. It might be possible to use union { char __c; T __unused; } and use addressof(__unused) for begin() but that increases the size of array<T, 0> to equal array<T, 1> and means a zero-sized array can never be a potentially overlapping subobject. But I agree that removing that requirement is non-editorial.

source/containers.tex Outdated Show resolved Hide resolved
Co-authored-by: Jonathan Wakely <github@kayari.org>
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

4 participants