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>: ranges::clamp, the projection may be applied at most three times. #1898

Merged
merged 18 commits into from
Jan 6, 2022

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented May 3, 2021

Fixes #1893

@fsb4000 fsb4000 requested a review from a team as a code owner May 3, 2021 08:53
stl/inc/algorithm Outdated Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
Co-authored-by: miscco <mschellenbergercosta@googlemail.com>
stl/inc/algorithm Show resolved Hide resolved
stl/inc/algorithm Outdated Show resolved Hide resolved
Co-authored-by: Michael Schellenberger Costa <mschellenbergercosta@googlemail.com>
Co-authored-by: statementreply <statementreply@gmail.com>
@fsb4000
Copy link
Contributor Author

fsb4000 commented May 3, 2021

Where should I add tests? At https://github.com/microsoft/STL/blob/main/tests/std/tests/P0896R4_ranges_alg_minmax/test.cpp or create new tests?

stl/inc/algorithm Outdated Show resolved Hide resolved
Co-authored-by: Michael Schellenberger Costa <mschellenbergercosta@googlemail.com>
Co-authored-by: statementreply <statementreply@gmail.com>
@miscco
Copy link
Contributor

miscco commented May 3, 2021

Where should I add tests? At https://github.com/microsoft/STL/blob/main/tests/std/tests/P0896R4_ranges_alg_minmax/test.cpp or create new tests?

Yes, that would be the best one. The "easiest" way would be to create a constexpr statefull projection and use that in the tests

Co-authored-by: Michael Schellenberger Costa <mschellenbergercosta@googlemail.com>
stl/inc/algorithm Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added bug Something isn't working ranges C++20/23 ranges labels May 3, 2021
@StephanTLavavej StephanTLavavej added this to Initial Review in Code Reviews via automation May 3, 2021
Code Reviews automation moved this from Initial Review to Work In Progress May 4, 2021
@CaseyCarter CaseyCarter self-assigned this May 4, 2021
fsb4000 and others added 2 commits May 5, 2021 07:34
Co-authored-by: Casey Carter <cartec69@gmail.com>
Co-authored-by: Casey Carter <cartec69@gmail.com>
@fsb4000
Copy link
Contributor Author

fsb4000 commented May 5, 2021

Thank you @Serikov ! I'm a little confused.
It seems gcc is just using two copies with no forward / move and no if constexpr for move-only types.
cppreference has "Possible implementation" with two forwards
https://en.cppreference.com/w/cpp/algorithm/ranges/clamp
If we determine that this is incorrect, then we can fix it on cppreference too.
Unfortunately, I do not yet understand which option is correct. I will wait for more opinions.

@Serikov
Copy link

Serikov commented May 6, 2021

@fsb4000 Sorry for the confusion. My understanding was wrong. Apparently that example contains precondition violation. More then that GCC clamp implementation is buggy. There is an answer on StackOverlow that clarifies why it is required to forward arguments twice (there is a link to an example in the comments).

@fsb4000 fsb4000 requested a review from CaseyCarter May 12, 2021 13:16
@StephanTLavavej StephanTLavavej moved this from Work In Progress to Initial Review in Code Reviews May 12, 2021
@StephanTLavavej

This comment has been minimized.

Code Reviews automation moved this from Initial Review to Work In Progress Dec 17, 2021
@CaseyCarter CaseyCarter changed the title <algorithm>: ranges::clamp, the projection may be applied at most three times. <algorithm>: ranges::clamp, the projection may be applied at most three times. Dec 17, 2021
fsb4000 and others added 2 commits December 17, 2021 23:28
Co-authored-by: Casey Carter <Casey@Carter.net>
@CaseyCarter CaseyCarter moved this from Work In Progress to Final Review in Code Reviews Dec 17, 2021
@CaseyCarter CaseyCarter removed their assignment Dec 17, 2021
@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Dec 17, 2021
@CaseyCarter CaseyCarter self-assigned this Jan 6, 2022
@CaseyCarter
Copy link
Member

I'm going to add this to the next batch of changes to merge - please notify me if any further commits are pushed.

@CaseyCarter CaseyCarter merged commit 582735a into microsoft:main Jan 6, 2022
Code Reviews automation moved this from Ready To Merge to Done Jan 6, 2022
@CaseyCarter
Copy link
Member

CaseyCarter commented Jan 6, 2022

Thank you for ensuring the number of projections in clamp are within bounds!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ranges C++20/23 ranges
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

<algorithm>: ranges::clamp doesn't meet the complexity requirements
7 participants