Skip to content

Implement ranges::starts_with and ranges::ends_with #1997

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
Merged
merged 27 commits into from
Oct 20, 2021
Merged

Implement ranges::starts_with and ranges::ends_with #1997

merged 27 commits into from
Oct 20, 2021
+612 −61

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Jun 10, 2021 •
edited by StephanTLavavej
Loading

This implements P1659 from @cjdb

I really wish we had something like an constexpr if-expression

Fixes #1975.

@miscco miscco requested a review from a team as a code owner June 10, 2021 11:56
@miscco
Copy link
Contributor Author

miscco commented Jun 10, 2021

Note I did not yet add the feature macro, what is the right value?

AdamBucior
AdamBucior suggested changes Jun 10, 2021
View reviewed changes
@StephanTLavavej StephanTLavavej added cxx23 C++23 feature ranges C++20/23 ranges labels Jun 10, 2021
@miscco
Copy link
Contributor Author

miscco commented Jun 11, 2021

@CaseyCarter it seems that mismatch was one of the first ranges algorithms so it has some oddities. I think we should move the unwrapping out of the helper functions and into the main function calls as in the other algorithms.

If so should that happen in this PR?

@StephanTLavavej

This comment has been minimized.

Sign in to view
AdamBucior
AdamBucior reviewed Jun 12, 2021
View reviewed changes
@miscco miscco force-pushed the P1659-ranges-starts_with branch from 03e4d37 to 75b4380 Compare June 12, 2021 12:50
@miscco
Copy link
Contributor Author

miscco commented Jun 12, 2021

I believe we should clean up _Mismatch_fn because currently we have a mismatch whether the algorithms expect uwrapped or plain iterators

@miscco
Copy link
Contributor Author

miscco commented Jun 22, 2021

@CaseyCarter, do you have some insights on what to do if the needle range is infinite?

Should that return true or false?

@miscco
Copy link
Contributor Author

miscco commented Jun 22, 2021

@AdamBucior I added the requested optimizations

@CaseyCarter
Copy link
Contributor

I really wish we had something like an constexpr if-expression

You mean something other than if constexpr and if consteval? I suspect EWG will be reluctant to add more variations of compiletime if.

Note I did not yet add the feature macro, what is the right value?

202106L

Because of my old Nemesis. Input iterators cannot be copied into distance

I'm working on #1684 now.

@CaseyCarter it seems that mismatch was one of the first ranges algorithms so it has some oddities. I think we should move the unwrapping out of the helper functions and into the main function calls as in the other algorithms.

I vaguely recall this being useful for some reason. Feel free to take a stab at it, and if something goes pearshaped we can add an explanatory comment so future us doesn't try to fix this again.

If so should that happen in this PR?

This PR won't be ported to 16.11, so I wouldn't complain about unrelated cleanup - your call.

I believe we should clean up _Mismatch_fn because currently we have a mismatch whether the algorithms expect uwrapped or plain iterators

Agree.

@CaseyCarter, do you have some insights on what to do if the needle range is infinite? Should that return true or false?

It should return false if a mismatch is found and otherwise run forever.

@StephanTLavavej

This comment has been minimized.

Sign in to view
StephanTLavavej
StephanTLavavej requested changes Jun 30, 2021
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

This PR was previously passing checks, but is now failing after the MegaMerge, with apparently-real failures with both MSVC and Clang in different tests.

@CaseyCarter CaseyCarter self-assigned this Jul 14, 2021
CaseyCarter
CaseyCarter suggested changes Sep 16, 2021
View reviewed changes
@miscco miscco force-pushed the P1659-ranges-starts_with branch from b1a7261 to 5f22bb6 Compare September 17, 2021 08:41
@CaseyCarter CaseyCarter self-requested a review September 29, 2021 21:27
miscco
miscco commented Oct 14, 2021
View reviewed changes
@CaseyCarter
Copy link
Contributor

@miscco FYI: I thought we could do better in ends_with than to iterate over both ranges twice when non-sized, so I implemented some special cases. (I may have gone overboard.)

@CaseyCarter CaseyCarter removed their assignment Oct 18, 2021
miscco
miscco commented Oct 18, 2021
View reviewed changes
StephanTLavavej
StephanTLavavej requested changes Oct 19, 2021
View reviewed changes
CaseyCarter
CaseyCarter reviewed Oct 19, 2021
View reviewed changes
stl/inc/algorithm
}
}

// distance(_First1, _Mid1) == distance(_First2, _Last2) == _Count2
Copy link
Contributor

Choose a reason for hiding this comment

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

Chained comparisons don't really work this way in C++ - shout if people think this comment is unclear.

Copy link
Member

Choose a reason for hiding this comment

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

I noticed it but decided not to meow. 🐱

@miscco
Copy link
Contributor Author

miscco commented Oct 19, 2021

Thanks for going the extra mile @CaseyCarter

Looks good to me

@StephanTLavavej StephanTLavavej self-assigned this Oct 19, 2021
@StephanTLavavej
Copy link
Member

I'm mirroring this to an MSVC-internal PR; please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit a9f1b27 into microsoft:main Oct 20, 2021
@StephanTLavavej
Copy link
Member

Thanks for implementing these new ranges algorithms! 🚀 😻 🛠️

@miscco miscco deleted the P1659-ranges-starts_with branch October 20, 2021 05:23
AZero13 pushed a commit to AZero13/STL that referenced this pull request Nov 4, 2021
@miscco @AdamBucior
@StephanTLavavej @CaseyCarter @AZero13
P1659R3 ranges::starts_with, ranges::ends_with (microsoft#1997)
c74c561 
Co-authored-by: Adam Bucior <[email protected]>
Co-authored-by: Stephan T. Lavavej <[email protected]>
Co-authored-by: Casey Carter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+2 more reviewers

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@AdamBucior AdamBucior AdamBucior approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@StephanTLavavej StephanTLavavej

Labels
cxx23 C++23 feature ranges C++20/23 ranges
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

P1659R3 ranges::starts_with, ranges::ends_with
4 participants
@miscco @StephanTLavavej @CaseyCarter @AdamBucior