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

<algorithm> Implement P2302R4 ranges::contains #2911

Merged
merged 8 commits into from
Jul 28, 2022

Conversation

SuperWig
Copy link
Contributor

@SuperWig SuperWig commented Jul 25, 2022

Fixes: #2921
I more or less just copied the tests for find and search. I'm guessing the feature test macro.

@SuperWig SuperWig requested a review from a team as a code owner July 25, 2022 17:46
@CaseyCarter CaseyCarter added ranges C++20/23 ranges cxx23 C++23 feature labels Jul 25, 2022
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
@SuperWig SuperWig changed the title <algorithm> Implement P2302R3 ranges::contains <algorithm> Implement P2302R4 ranges::contains Jul 25, 2022
tests/std/tests/P2302R3_ranges_alg_contains/env.lst Outdated Show resolved Hide resolved
stl/inc/yvals_core.h Outdated Show resolved Hide resolved
stl/inc/algorithm Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
requires indirect_binary_predicate < ranges::equal_to, projected<_It, _Pj>,
const _Ty* > //
_NODISCARD constexpr bool operator()(_It _First, _Se _Last, const _Ty& _Val, _Pj _Proj = {}) const {
return _RANGES find(_STD move(_First), _Last, _Val, _Pass_fn(_Proj)) != _Last;
Copy link
Member

Choose a reason for hiding this comment

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

No change requested: there are _Pass_fn calls at multiple layers here, which we usually avoid, but I checked and _Pass_fn will not wrap twice, so this seems reasonable for now.

tests/std/tests/P2302R3_ranges_alg_contains/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2302R3_ranges_alg_contains/env.lst Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks! I validated and pushed minor fixes/cleanups, the most significant of which was guarding the product code for C++23.

@StephanTLavavej StephanTLavavej self-assigned this Jul 27, 2022
@StephanTLavavej
Copy link
Member

I'm speculatively mirroring this to the MSVC-internal repo (because I'm a greedy kitty who likes treats) - please notify me if any further changes are pushed.

@SuperWig
Copy link
Contributor Author

@StephanTLavavej pushed a minor change to the contains_subrange test to constrain the template params to forward ranges.

@StephanTLavavej
Copy link
Member

@SuperWig Updated the mirror, thanks for letting me know! 😻

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

LGTM!
image

Copy link
Contributor

@miscco miscco left a comment

Choose a reason for hiding this comment

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

That is a great contribution

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

miscco commented Jul 27, 2022

@StephanTLavavej I think there are some issues that should be addressed

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
requires indirect_binary_predicate<ranges::equal_to, projected<iterator_t<_Rng>, _Pj>, const _Ty*>
_NODISCARD constexpr bool operator()(_Rng&& _Range, const _Ty& _Val, _Pj _Proj = {}) const {
// clang-format on
const auto _UResult = _RANGES _Find_unchecked(_Ubegin(_Range), _Uend(_Range), _Val, _Pass_fn(_Proj));
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we should memoize _Uend(_Range) as calling it twice might be expensive

Copy link
Member

@CaseyCarter CaseyCarter Jul 27, 2022

Choose a reason for hiding this comment

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

We generally don't avoid repeated calls to begin and end, because they are generally O(1). The first call may be expensive, but the subsequent calls are not. (Caching things across big function calls consumes stack and pushes us closer to cold stack space, so it's not a no-brainer.)

stl/inc/algorithm Outdated Show resolved Hide resolved
return true;
}

const auto _Match = _RANGES search(_Range1, _Range2, _Pass_fn(_Pred), _Pass_fn(_Proj1), _Pass_fn(_Proj2));
Copy link
Contributor

@miscco miscco Jul 27, 2022

Choose a reason for hiding this comment

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

Should we forward _Range1 and move _Range2

Copy link
Member

Choose a reason for hiding this comment

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

lvalue ranges are strictly more capable than xvalue ranges, so forwarding is just noise for the purposes of algorithms.

@StephanTLavavej
Copy link
Member

Thanks, updating mirror! 🪞

@StephanTLavavej StephanTLavavej merged commit f8a9c6f into microsoft:main Jul 28, 2022
@StephanTLavavej
Copy link
Member

Thanks for implementing this C++23 ranges feature - I can barely contain my excitement! 😹 🎉 😻

@SuperWig
Copy link
Contributor Author

Glad to be part of bringing this super advanced technology that took decades to standardise.

@SuperWig SuperWig deleted the contains branch July 28, 2022 07:09
fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
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.

P2302R4 ranges::contains, ranges::contains_subrange
5 participants