Skip to content

Conversation

miscco
Copy link
Contributor

@miscco miscco commented Jul 3, 2021

Fixes #1985

I am still baffled why array<string, 5>{{{}, "Hello ", {}, "World!", {}}} | views::join was not fixed 😿

@miscco miscco requested a review from a team as a code owner July 3, 2021 12:02
@miscco miscco force-pushed the P2328-join-view branch from 433caa3 to 92ce96e Compare July 3, 2021 12:03
@miscco
Copy link
Contributor Author

miscco commented Jul 3, 2021

Derp, I did not update the can_test boolean that enables the test...

@miscco miscco force-pushed the P2328-join-view branch from 150a851 to 49bf9a3 Compare July 3, 2021 18:56
Copy link
Contributor

@timsong-cpp timsong-cpp left a comment

Choose a reason for hiding this comment

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

I am still baffled why array<string, 5>{{{}, "Hello ", {}, "World!", {}}} | views::join was not fixed 😿

I see nothing to fix, other than perhaps making array a view?

I also note that the paper title is incorrectly transcribed.

@StephanTLavavej StephanTLavavej added cxx20 C++20 feature defect report Applied retroactively ranges C++20/23 ranges labels Jul 3, 2021
@CaseyCarter CaseyCarter self-assigned this Jul 14, 2021
@CaseyCarter CaseyCarter changed the title Implement P2328R1 join_view should join all ranges Implement P2328R1 join_view Should Join All Views Of Ranges Jul 15, 2021
@CaseyCarter
Copy link
Contributor

CaseyCarter commented Jul 15, 2021

I also note that the paper title is incorrectly transcribed.

I'm tempted to veto the Stephan-casing of the title; "Views of Ranges" doesn't have the same meaning as the intended "views of ranges". @StephanTLavavej, could you live with "P2328R11 join_view Should Join All views Of ranges"?

@CaseyCarter CaseyCarter assigned miscco and unassigned CaseyCarter Jul 15, 2021
Co-authored-by: Casey Carter <[email protected]>
@StephanTLavavej StephanTLavavej self-assigned this Aug 4, 2021
@CaseyCarter CaseyCarter removed their assignment Aug 5, 2021
@CaseyCarter
Copy link
Contributor

@miscco FYI I pushed a change that removed all of the _Dummy members from the optional-alike types in <ranges>. We don't need them in C++20, since we can get away with initializing no union member even in a constexpr constructor.

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.

Looks good to me, after @timsong-cpp's comments are addressed.

@miscco
Copy link
Contributor Author

miscco commented Aug 5, 2021

@timsong-cpp I think I managed to get it working as you suggested.

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.

Looks good, I'll push a trivial change to take (int) instead of (const int).

@StephanTLavavej StephanTLavavej removed their assignment Aug 6, 2021
@StephanTLavavej
Copy link
Member

FYI @CaseyCarter, @miscco and I pushed changes after you approved.

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

Mirrored to internal MSVC-PR-343332. Please notify me if any further changes are pushed to this branch.

@StephanTLavavej
Copy link
Member

@miscco @CaseyCarter This failed in the internal test harness so I had to drop the || defined(MSVC_INTERNAL_TESTING) logic.

@StephanTLavavej StephanTLavavej merged commit 7f4cc9e into microsoft:main Aug 10, 2021
@StephanTLavavej
Copy link
Member

Thanks for joining this project! 😹 🎉 🚀

@StephanTLavavej StephanTLavavej removed their assignment Aug 11, 2021
@miscco miscco deleted the P2328-join-view branch August 11, 2021 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx20 C++20 feature defect report Applied retroactively ranges C++20/23 ranges
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P2328R1 join_view Should Join All views Of ranges
6 participants