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

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

Merged
merged 27 commits into from
Oct 20, 2021

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Jun 10, 2021

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?

stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the cxx23 C++23 feature label Jun 10, 2021
@StephanTLavavej StephanTLavavej added the ranges C++20/23 ranges label Jun 11, 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.

stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
@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
Member

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.

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
stl/inc/algorithm Outdated Show resolved Hide resolved
@CaseyCarter
Copy link
Member

@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
stl/inc/algorithm Outdated Show resolved Hide resolved
tests/std/tests/P1659R3_ranges_alg_starts_with/test.cpp Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
}
}

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

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
AreaZR pushed a commit to AreaZR/STL that referenced this pull request Nov 4, 2021
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
Labels
cxx23 C++23 feature ranges C++20/23 ranges
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P1659R3 ranges::starts_with, ranges::ends_with
4 participants