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

Use _Invoke_result_t<_Urng&> instead of _Urng::result_type in _Rng_from_urng #3002

Merged
merged 4 commits into from
Aug 9, 2022

Conversation

cpplearner
Copy link
Contributor

C++20 uniform_random_bit_generator does not require the presence of member result_type.

This is tested in libc++ tests std/algorithms/alg.modifying.operations/alg.random.shuffle/ranges_shuffle.pass.cpp and std/algorithms/alg.modifying.operations/alg.random.sample/ranges_sample.pass.cpp. Unfortunately they fail even after this change, because they assume that std::array iterators are pointers.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Aug 7, 2022
stl/inc/xutility Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

Thanks for finding and fixing this bug! I've pushed a fix for value categories with a corresponding test change (fails without the fix, passes with it).

@MattStephanson
Copy link
Contributor

C++20 uniform_random_bit_generator does not require the presence of member result_type

That's true for the concept, but doesn't [rand.req.urng]/3 require "G provides a nested typedef-name result_­type that denotes the same type as invoke_­result_­t<G&>"? These generators in libc++ meet the URBG concept, but not the named requirements.

@cpplearner
Copy link
Contributor Author

C++20 uniform_random_bit_generator does not require the presence of member result_type

That's true for the concept, but doesn't [rand.req.urng]/3 require "G provides a nested typedef-name result_­type that denotes the same type as invoke_­result_­t<G&>"? These generators in libc++ meet the URBG concept, but not the named requirements.

Old std::shuffle requires the generator type to meet the uniform random bit generator named requirements (and thus requires member result_type), but C++20 ranges::shuffle only requires the type to model the concept.

Since ranges::shuffle uses _Rng_from_urng, the latter must not rely on result_type.

If we want to be strict, we could change std::shuffle to check the presence of result_type.

@MattStephanson
Copy link
Contributor

MattStephanson commented Aug 8, 2022 via email

@@ -5705,12 +5705,12 @@ template <class _Diff, class _Urng>
class _Rng_from_urng { // wrap a URNG as an RNG
public:
using _Ty0 = make_unsigned_t<_Diff>;
using _Ty1 = typename _Urng::result_type;
using _Ty1 = _Invoke_result_t<_Urng&>;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't _Invoke_result_t<_Urng&> a bit heavyweight when we could simply decltype(_STD declval<_Urng&>()())?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but I don't think that this will ever be frequently instantiated in a program - so I'm not concerned about throughput here.

@StephanTLavavej StephanTLavavej self-assigned this Aug 9, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej changed the title Use _Invoke_result_t<_Urng> instead of _Urng::result_type in _Rng_from_urng Use _Invoke_result_t<_Urng&> instead of _Urng::result_type in _Rng_from_urng Aug 9, 2022
@StephanTLavavej StephanTLavavej merged commit ced5cde into microsoft:main Aug 9, 2022
@StephanTLavavej
Copy link
Member

Thanks for randomly fixing this bug! 🐞 🛠️ 😹

fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022
…ng_from_urng` (microsoft#3002)

Co-authored-by: Stephan T. Lavavej <stl@nuwen.net>
@cpplearner cpplearner deleted the invoke_result branch August 25, 2022 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants