Skip to content

Implement P2328R1 join_view Should Join All Views Of Ranges - #2038

Merged
Stephan T. Lavavej (StephanTLavavej) merged 22 commits into
microsoft:mainfrom
miscco:P2328-join-view
Aug 10, 2021
Merged

Implement P2328R1 join_view Should Join All Views Of Ranges#2038
Stephan T. Lavavej (StephanTLavavej) merged 22 commits into
microsoft:mainfrom
miscco:P2328-join-view

Conversation

@miscco

@miscco Michael Schellenberger Costa (miscco) commented Jul 3, 2021

Copy link
Copy Markdown
Contributor

Fixes #1985

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

@miscco

Michael Schellenberger Costa (miscco) commented Jul 3, 2021

Copy link
Copy Markdown
Contributor Author

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

AdamBucior
Adam Bucior (AdamBucior) reviewed Jul 3, 2021
View reviewed changes
Comment thread stl/inc/ranges
miscco
Michael Schellenberger Costa (miscco) commented Jul 3, 2021
View reviewed changes
Comment thread stl/inc/ranges Outdated
miscco
Michael Schellenberger Costa (miscco) commented Jul 3, 2021
View reviewed changes
Comment thread stl/inc/ranges Outdated
miscco
Michael Schellenberger Costa (miscco) commented Jul 3, 2021
View reviewed changes
Comment thread stl/inc/ranges Outdated
AdamBucior
Adam Bucior (AdamBucior) reviewed Jul 3, 2021
View reviewed changes
Comment thread stl/inc/ranges Outdated
Comment thread stl/inc/ranges Outdated
timsong-cpp
timsong-cpp suggested changes Jul 3, 2021
View reviewed changes

@timsong-cpp timsong-cpp left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread stl/inc/ranges Outdated
Comment thread stl/inc/ranges Outdated
Comment thread stl/inc/ranges Outdated
Comment thread stl/inc/ranges Outdated
@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) added cxx20 C++20 feature defect report Applied retroactively ranges C++20/23 ranges labels Jul 3, 2021
@miscco
More test coverage and internal testing
351f538
@miscco @AdamBucior @tcanens
Address more review comments
72f392c 
Co-authored-by: Adam Bucior <35536269+AdamBucior@users.noreply.github.com>
Co-authored-by: Tim Song <t.canens.cpp@gmail.com>
eldakesh-ms
Elnar Dakeshov (eldakesh-ms) reviewed Jul 14, 2021
View reviewed changes
Comment thread stl/inc/ranges Outdated
CaseyCarter
Casey Carter (CaseyCarter) suggested changes Jul 15, 2021
View reviewed changes
Comment thread stl/inc/ranges Outdated
Comment thread stl/inc/ranges Outdated
Comment thread stl/inc/ranges
Comment thread stl/inc/ranges Outdated
Comment thread stl/inc/ranges Outdated
Comment thread stl/inc/ranges Outdated
Comment thread stl/inc/ranges Outdated
Comment thread stl/inc/ranges Outdated
Comment thread tests/std/tests/P0896R4_views_join/test.cpp Outdated
Comment thread tests/std/tests/P0896R4_views_join/test.cpp Outdated
@CaseyCarter Casey Carter (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

Casey Carter (CaseyCarter) commented Jul 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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". Stephan T. Lavavej (@StephanTLavavej), could you live with "P2328R11 join_view Should Join All views Of ranges"?

CaseyCarter
Casey Carter (CaseyCarter) reviewed Jul 15, 2021
View reviewed changes
Comment thread stl/inc/ranges Outdated
@miscco @CaseyCarter
More review comments
c2bc106 
Co-authored-by: Casey Carter <cacarter@microsoft.com>
CaseyCarter
Casey Carter (CaseyCarter) reviewed Aug 4, 2021
View reviewed changes
Comment thread tests/std/tests/P0896R4_views_join/test.cpp Outdated
Comment thread stl/inc/ranges Outdated
Comment thread tests/std/tests/P0896R4_views_join/test.cpp Outdated
@CaseyCarter Casey Carter (CaseyCarter) removed their assignment Aug 5, 2021
@CaseyCarter

Casey Carter (CaseyCarter) commented Aug 5, 2021

Copy link
Copy Markdown
Contributor

Michael Schellenberger Costa (@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.

timsong-cpp
timsong-cpp reviewed Aug 5, 2021
View reviewed changes
Comment thread stl/inc/ranges Outdated
Comment thread stl/inc/ranges Outdated
Comment thread tests/std/tests/P0896R4_views_join/test.cpp
StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) requested changes Aug 5, 2021
View reviewed changes

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) left a comment

Copy link
Copy Markdown
Member

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

Michael Schellenberger Costa (miscco) commented Aug 5, 2021

Copy link
Copy Markdown
Contributor Author

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

timsong-cpp
timsong-cpp reviewed Aug 5, 2021
View reviewed changes
Comment thread stl/inc/ranges Outdated
StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) reviewed Aug 6, 2021
View reviewed changes

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) left a comment

Copy link
Copy Markdown
Member

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).

Comment thread tests/std/tests/P0896R4_views_join/test.cpp Outdated
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Aug 7, 2021

Copy link
Copy Markdown
Member

FYI Casey Carter (@CaseyCarter), Michael Schellenberger Costa (@miscco) and I pushed changes after you approved.

@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Aug 9, 2021

Copy link
Copy Markdown
Member

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

@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Aug 9, 2021

Copy link
Copy Markdown
Member

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

@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit 7f4cc9e into microsoft:main Aug 10, 2021
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Aug 10, 2021

Copy link
Copy Markdown
Member

Thanks for joining this project! 😹 🎉 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) StephanTLavavej approved these changes
+4 more reviewers
@timsong-cpp timsong-cpp timsong-cpp requested changes
@AdamBucior Adam Bucior (AdamBucior) AdamBucior left review comments
@eldakesh-ms Elnar Dakeshov (eldakesh-ms) eldakesh-ms left review comments
@CaseyCarter Casey Carter (CaseyCarter) CaseyCarter approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cxx20 C++20 feature defect report Applied retroactively ranges C++20/23 ranges

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

P2328R1 join_view Should Join All views Of ranges

6 participants

@miscco @CaseyCarter @StephanTLavavej @timsong-cpp @AdamBucior @eldakesh-ms