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

Applied LLVM-47414 workaround to std::ranges::join_view #2352

Merged
merged 6 commits into from Dec 9, 2021

Conversation

AnabelSMRuggiero
Copy link
Contributor

When piping into std::views::join, Clang eagerly instantiates the join_view::_InnerRng template alias. This alias is used in the requirements of the resulting view's const qualified begin() and end() methods. This check is performed and causes a hard compile error even if the source contains no calls to the const qualified methods.

I used the workaround applied to the same methods of std::views::transform_view.

A simplified reproduction of code that fails to compile under Clang can be found here. Piping a filter_view or a view composed on top of a filter_view will cause this erroneous compile error; filter_view has no const qualified overloads of begin() and end().

@AnabelSMRuggiero AnabelSMRuggiero requested a review from a team as a code owner November 22, 2021 15:49
@ghost
Copy link

ghost commented Nov 22, 2021

CLA assistant check
All CLA requirements met.

@CaseyCarter CaseyCarter added bug Something isn't working ranges C++20/23 ranges labels Nov 22, 2021
@CaseyCarter CaseyCarter added this to Initial Review in Code Reviews via automation Nov 22, 2021
stl/inc/ranges Show resolved Hide resolved
stl/inc/ranges Show resolved Hide resolved
stl/inc/ranges Outdated Show resolved Hide resolved
Code Reviews automation moved this from Initial Review to Work In Progress Nov 22, 2021
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

The fix looks great, but we're missing a regression test. Could you please add your minimal repro as a test case in tests/std/tests/P0896R4_views_join/test.cpp?

@AnabelSMRuggiero
Copy link
Contributor Author

I added most of the requested changes in a new branch of my fork.
I wanted to delay pushing the changes to my main branch while waiting for a confirmation about whether to maintain consistency with the transform_view workaround. Figured it'd be worth making them visible in some way while avoiding triggering an extra run of CI; I have tried out the new test case added for join_view locally.

stl/inc/ranges Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter moved this from Work In Progress to Final Review in Code Reviews Nov 22, 2021
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.

Thanks! I'll go ahead and push changes for these trivial code review comments.

tests/std/tests/P0896R4_views_join/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Nov 23, 2021
@StephanTLavavej StephanTLavavej self-assigned this Dec 8, 2021
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 530db31 into microsoft:main Dec 9, 2021
Code Reviews automation moved this from Ready To Merge to Done Dec 9, 2021
@StephanTLavavej
Copy link
Member

Thanks for improving join_view on Clang - and congratulations on your first microsoft/STL pull request! 🎉 😸 🚀

This will ship in VS 2022 17.2 Preview 2.

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.

None yet

4 participants