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.

Sign up for GitHub

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

Jump to bottom

[span.iterators] Specify aliases, like [string.view.iterators] does #4062

Merged
merged 1 commit into from Sep 5, 2020
Merged

[span.iterators] Specify aliases, like [string.view.iterators] does #4062

merged 1 commit into from Sep 5, 2020

Conversation

JohelEGP
Copy link
Contributor

No description provided.

CaseyCarter
CaseyCarter suggested changes Jun 28, 2020
View reviewed changes
source/containers.tex Outdated
@@ -11236,7 +11236,10 @@
requirements\iref{random.access.iterators},
and
meets the requirements for
constexpr iterators\iref{iterator.requirements.general}.
constexpr iterators\iref{iterator.requirements.general},
whose \tcode{value_type} is the template parameter \tcode{ElementType}.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be remove_cv_t<ElementType>; value types are not cv-qualified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to resolve these comments, @JohelEGP. I can't since I'm a happily powerless contributor here =)

CaseyCarter
CaseyCarter reviewed Jun 28, 2020
View reviewed changes
source/containers.tex Outdated
Comment on lines 11240 to 11241
whose \tcode{value_type} is \tcode{remove_cv_t<ElementType>} and
whose \tcode{reference_type} is \tcode{ElementType&}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since span has nested types value_type and reference, why not simply use those? (i.e., whose value type is \tcode{value_type} and whose reference type is \tcode{reference})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent!

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Jun 28, 2020 •
edited

I was trying to specify what seems underspecified, which I noticed when looking at string_view::const_iterator. Perhaps the problem lies elsewhere and an LWG issue is required.

Now I'm left wondering if the reference of string_view::const_iterator is underspecified. They specify that "All requirements on container iterators ([container.requirements]) apply to the-view-type's-iterator as well", and [container.requirements]p6 says that "begin() returns an iterator referring to the first element in the container", but neither view is a container.

@JohelEGP JohelEGP changed the title [span.iterators] Specify value_type, like [string.view.iterators] [span.iterators] Specify aliases, like [string.view.iterators] does Jun 28, 2020
@jensmaurer
Copy link
Member

The commit description seems broken. "specify aliases" feels misleading.
Maybe "Specify iterator value_type and reference"

And please squash all commits and force-push.

@jensmaurer jensmaurer added the changes requested Changes to the wording or approach have been requested and not yet applied. label Sep 3, 2020
CaseyCarter
CaseyCarter reviewed Sep 4, 2020
View reviewed changes
source/containers.tex Outdated
Comment on lines 11240 to 11241
whose \tcode{value_type} is \tcode{value_type} and
whose \tcode{reference} type is \tcode{reference}.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear what entities \tcode{value_type} and \tcode{reference} refer to here. Presumably we mean member type aliases of iterator_traits<iterator>, so we could say such that \tcode{iterator_traits<iterator>::value_type} is \tcode{value_type} (and similarly for reference) but I think it would be clearer to simply use prose "value type" and "reference type" which name those properties of iterators:

Suggested change
whose \tcode{value_type} is \tcode{value_type} and
whose \tcode{reference} type is \tcode{reference}.
whose value type is \tcode{value_type} and
whose reference type is \tcode{reference}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good, but deviates from [string.view.iterators], which uses value_type. If the editors prefer this, I'd also like to extend the PR to [string.view.iterators] so that it also reads "value type".

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that would be an improvement. Having \tcode{value_type} mean different things two words apart is not good.

jensmaurer
jensmaurer requested changes Sep 4, 2020
View reviewed changes
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.

Please follow Casey's suggestion.

source/containers.tex Outdated
Comment on lines 11240 to 11241
whose \tcode{value_type} is \tcode{value_type} and
whose \tcode{reference} type is \tcode{reference}.
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that would be an improvement. Having \tcode{value_type} mean different things two words apart is not good.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Sep 4, 2020

Decided against my suggested change to [string.view.iterators].

@jensmaurer jensmaurer removed the changes requested Changes to the wording or approach have been requested and not yet applied. label Sep 4, 2020
@jensmaurer
Copy link
Member

@CaseyCarter , are you ok with this end result?

CaseyCarter
CaseyCarter approved these changes Sep 5, 2020
View reviewed changes
Copy link
Contributor

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

LGTM!

@jensmaurer jensmaurer merged commit 9cc50c1 into cplusplus:master Sep 5, 2020
@JohelEGP JohelEGP deleted the span.iterators branch September 5, 2020 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@jensmaurer jensmaurer jensmaurer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@JohelEGP @jensmaurer @CaseyCarter