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

[span.iterators] Fix cross-reference to container iterators #6183

Merged
merged 2 commits into from Mar 14, 2023

Conversation

AlisdairM
Copy link
Contributor

The current cross-reference is to [containers.requirements], which is the whole containers requirements clause, including not just general containers, but also allocator-aware, reversible, sequence, associative, and unodered associative containers. It seems very unlikely that the cross-reference expects to be the intersection of all of those.

Rather, the reference seems to intend just the [containers.reqmts] subclause, which adds just two useful constraints: random access iterators should support the 3-way comparison operator, and the interoperabiity of container iterators and const_iterators.

As a drive-by, fixed the \indexlibrarymember macro by ordering the type, span, before the member, iterator, as is the convention. This has no impact on the index, as entries in both directions are created. However, it makes the wording of index macros across the clause consistent.

The current cross-reference is to [containers.requirements], which
is the whole containers requirements clause, including not just
general containers, but also allocator-aware, reversible, sequence,
associative, and unodered associative containers.  It seems very
unlikely that the cross-reference expects to be the intersection
of all of those.

Rather, the reference seems to intend just the [containers.reqmts]
subclause, which adds just two useful constraints: random access
iterators should support the 3-way comparison operator, and the
interoperabiity of container iterators and const_iterators.

As a drive-by, fixed the \indexlibrarymember macro by ordering
the type, span, before the member, iterator, as is the convention.
This has no impact on the index, as entries in both directions are
created.  However, it makes the wording of index macros across the
clause consistent.
@AlisdairM
Copy link
Contributor Author

Looking to @CaseyCarter to confirm cross-ref change is correct, and @jwakely to confirm this change is editorial.

@JohelEGP
Copy link
Contributor

Rather, the reference seems to intend just the [containers.reqmts] subclause, which adds just two useful constraints: random access iterators should support the 3-way comparison operator, and the interoperabiity of container iterators and const_iterators.

If that's the case, there's another place equally affected:

$ git grep -C1 'container iterators' *.tex
containers.tex-\pnum
containers.tex:All requirements on container iterators\iref{container.requirements} apply to
containers.tex-\tcode{span::iterator} as well.
--
strings.tex-\pnum
strings.tex:All requirements on container iterators\iref{container.requirements} apply to \tcode{basic_string_view::const_iterator} as well.
strings.tex-\end{itemdescr}

@JohelEGP
Copy link
Contributor

JohelEGP commented Mar 14, 2023

There's more similarly affected:

$ git grep 'using iterator.*container.requirements' *.tex
containers.tex:    using iterator               = @\impdefx{type of \tcode{array::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{deque::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator        = @\impdefx{type of \tcode{forward_list::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{list::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{vector::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{vector<bool>::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{map::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{multimap::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{set::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{multiset::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator             = @\impdefx{type of \tcode{unordered_map::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator             = @\impdefx{type of \tcode{unordered_multimap::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator             = @\impdefx{type of \tcode{unordered_set::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator             = @\impdefx{type of \tcode{unordered_multiset::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{flat_map::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{flat_multimap::iterator}}@;     // see \ref{container.requirements}
containers.tex:    using iterator                  = @\impdefx{type of \tcode{flat_set::iterator}}@;  // see \ref{container.requirements}
containers.tex:    using iterator                  = @\impdefx{type of \tcode{flat_multiset::iterator}}@;  // see \ref{container.requirements}
strings.tex:    using iterator               = @\impdefx{type of \tcode{basic_string::iterator}}@; // see \ref{container.requirements}

@jensmaurer
Copy link
Member

Yes, would be nice to fix them all.

As a drive-by, fixed the \indexlibrarymember macro by ordering the type, span, before the member, iterator, as is the convention.

Not really; please "grep indexlibrarymember" and you'll see the convention is usually "member, type". Please split this off into a separate commit or a separate pull request if you continue to feel this needs alignment.

@@ -18364,7 +18364,7 @@

\rSec4[span.iterators]{Iterator support}

\indexlibrarymember{iterator}{span}%
\indexlibrarymember{span}{iterator}%
Copy link
Member

Choose a reason for hiding this comment

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

This is not what we usually do (globally); please split off into a separate commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted this drive-by, no separate commit forthcoming at this time.

@AlisdairM
Copy link
Contributor Author

AlisdairM commented Mar 14, 2023

There's more similarly affected:

$ git grep 'using iterator.*container.requirements' *.tex
containers.tex:    using iterator               = @\impdefx{type of \tcode{array::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{deque::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator        = @\impdefx{type of \tcode{forward_list::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{list::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{vector::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{vector<bool>::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{map::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{multimap::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{set::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{multiset::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator             = @\impdefx{type of \tcode{unordered_map::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator             = @\impdefx{type of \tcode{unordered_multimap::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator             = @\impdefx{type of \tcode{unordered_set::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator             = @\impdefx{type of \tcode{unordered_multiset::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{flat_map::iterator}}@; // see \ref{container.requirements}
containers.tex:    using iterator               = @\impdefx{type of \tcode{flat_multimap::iterator}}@;     // see \ref{container.requirements}
containers.tex:    using iterator                  = @\impdefx{type of \tcode{flat_set::iterator}}@;  // see \ref{container.requirements}
containers.tex:    using iterator                  = @\impdefx{type of \tcode{flat_multiset::iterator}}@;  // see \ref{container.requirements}
strings.tex:    using iterator               = @\impdefx{type of \tcode{basic_string::iterator}}@; // see \ref{container.requirements}

Actually, these cases are slightly different. Span is not a container, so it is unclear which of the various container requirements are intended to apply. All of these other cases are containers, and the category of container is known, so the whole subclause does apply additional constraints.

I could apply the broader edit if requested, but suggest that should be under a different ticket as the reasoning is more involved.

Copy link
Member

@jensmaurer jensmaurer left a comment

Choose a reason for hiding this comment

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

squash before merge

@tkoeppe tkoeppe merged commit dcac5ea into cplusplus:main Mar 14, 2023
2 checks passed
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