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

[string.view.template][string.view.iterators] Move requirements to a more appropriate place #2865

Merged
merged 1 commit into from Jun 12, 2019
Merged

[string.view.template][string.view.iterators] Move requirements to a more appropriate place #2865

merged 1 commit into from Jun 12, 2019

Conversation

JohelEGP
Copy link
Contributor

@JohelEGP JohelEGP commented May 3, 2019 •
edited

Moved down constexpr iterator requirement with the other iterator requirements, and moved up the invalidation requirements alongside the complexity requirements as both apply as generally.

@JohelEGP JohelEGP changed the title [string.view.template][string.view.iterators] Gather iterator require… [string.view.template][string.view.iterators] Move requirements to a more appropriate place May 3, 2019
@JohelEGP JohelEGP force-pushed the string_view branch 2 times, most recently from cee0dcd to b316fbc Compare May 3, 2019 19:06
jensmaurer
jensmaurer requested changes May 3, 2019
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.

I think the exclusion of *this is not obviously editorial, but will need a nod by LWG.

The move of the invalidation rule looks good to me in general. Where is it placed for std::string, for comparison?

In order to make quick(er) progress here, please prepare a pull request with just the text move, not related to #2866.

@jensmaurer jensmaurer added the changes requested Changes to the wording or approach have been requested and not yet applied. label May 3, 2019
@JohelEGP
Copy link
Contributor Author

JohelEGP commented May 3, 2019

Right away.

Where is it placed for std::string, for comparison?

http://wg21.link/string.require#4 and presumably subsumed from http://wg21.link/basic.string#2.

@jensmaurer
Copy link
Member

jensmaurer commented May 3, 2019 •
edited

Thanks for the updates. That's progress.

Uh, the automated checks fail. Maybe there's a surplus space at the end of a line, or an overfull hbox. Could you have a look, please?

@JohelEGP
Copy link
Contributor Author

JohelEGP commented May 3, 2019

Before:
1556918504
After:
1556918345
Seems like the () at the end. How do I solve this?

@JohelEGP
Copy link
Contributor Author

JohelEGP commented May 3, 2019

Now it looks alright.
1556922829

@jensmaurer jensmaurer removed the changes requested Changes to the wording or approach have been requested and not yet applied. label May 3, 2019
jensmaurer
jensmaurer approved these changes May 3, 2019
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.

Looks good now. Thanks for the work. (I'd need a second reviewer to approve before I can commit.)

@jensmaurer jensmaurer requested a review from jwakely May 3, 2019 23:00
@zygoloid zygoloid merged commit dd7b519 into cplusplus:master Jun 12, 2019
@JohelEGP JohelEGP deleted the string_view branch June 13, 2019 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jensmaurer jensmaurer jensmaurer approved these changes

@jwakely jwakely Awaiting requested review from jwakely

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