Skip to content

ranges::distance fails to emulate the Standard's two overload presentation - #2987

Merged
Stephan T. Lavavej (StephanTLavavej) merged 5 commits into
microsoft:mainfrom
CaseyCarter:distance-fix
Aug 27, 2022
Merged

ranges::distance fails to emulate the Standard's two overload presentation#2987
Stephan T. Lavavej (StephanTLavavej) merged 5 commits into
microsoft:mainfrom
CaseyCarter:distance-fix

Conversation

@CaseyCarter

@CaseyCarter Casey Carter (CaseyCarter) commented Aug 3, 2022

Copy link
Copy Markdown
Contributor

By not properly constraining itself with the union of the constraints of those two overloads. If we weren't trying to do something fancy just to avoid a bit of non-subsumption concept overloading, we wouldn't have written this bug in the first place. Let's split out into two overloads.

Test coverage is incoming in the LLVM update, against which I've verified the fix.

Merge conflict with #3024

…ion of LWG-3664

By constraining itself with the union of the constraints of those two overloads. Test coverage is incoming in the LLVM update, against which I've verified the fix.
@CaseyCarter Casey Carter (CaseyCarter) added bug Something isn't working ranges C++20/23 ranges labels Aug 3, 2022
@CaseyCarter
Casey Carter (CaseyCarter) requested a review from a team as a code owner August 3, 2022 16:20
@strega-nil-ms

Copy link
Copy Markdown
Contributor

Is there a reason not to just do the two-overload version, given that the two-overload version is how the standard is written?

@CaseyCarter

Copy link
Copy Markdown
Contributor Author

Is there a reason not to just do the two-overload version, given that the two-overload version is how the standard is written?

There are exactly two reasons why an STL implementation would take advantage of as-if to do something differently from how it's phrased in the Standard: performance and throughput. This is the latter.

We generally prefer if constexpr dispatch to concept overloading because it's quicker, even in cases like this where subsumption isn't a factor (normalization and subsumption checking are the really expensive bits). That said, I could be convinced that this bug wouldn't have happened if I hadn't tried to be fancy, and that I should fix the wording in LWG-3664 if I want to do something different.

@strega-nil-ms

Copy link
Copy Markdown
Contributor

I would agree with the latter point. If they took the same parameters, it would be different IMO, but doing this weird decay_t stuff to allow taking either a const I& OR an I by value is a lot.

@CaseyCarter Casey Carter (CaseyCarter) changed the title ranges::distance needs to better emulate the two overload presentation of LWG-3664 ranges::distance fails to emulate the Standard's two overload presentation Aug 3, 2022
@CaseyCarter

Copy link
Copy Markdown
Contributor Author

I would agree with the latter point. If they took the same parameters, it would be different IMO, but doing this weird decay_t stuff to allow taking either a const I& OR an I by value is a lot.

Split into two overloads, reverified against the incoming libc++ test, and rewrote the commit message.

Comment thread stl/inc/xutility Outdated
Comment thread stl/inc/xutility
Comment thread tests/std/tests/P0896R4_ranges_iterator_machinery/test.cpp Outdated
@StephanTLavavej

Copy link
Copy Markdown
Member

Looks good to me. FYI nicole mazzuca (@strega-nil-ms), I pushed a couple of comment changes after you approved.

@CaseyCarter

Copy link
Copy Markdown
Contributor Author

Thanks for finishing this up, nicole mazzuca (@strega-nil-ms)!

@StephanTLavavej

Copy link
Copy Markdown
Member

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

@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit 7cdd700 into microsoft:main Aug 27, 2022
@StephanTLavavej

Copy link
Copy Markdown
Member

Thanks for going the distance here! 🏃 🥇 😹

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

None yet

Development

Successfully merging this pull request may close these issues.

5 participants