This is an archive of the discontinued LLVM Phabricator instance.

[libc++][ranges] P2711R1 Making multi-param constructors of views explicit
ClosedPublic

Authored by H-G-Hristov on Feb 26 2023, 2:56 AM.

Details

Summary

Implemented P2711R1 for existing views.
(join_with_view is not yet implemented)

Diff Detail

Event Timeline

H-G-Hristov created this revision.Feb 26 2023, 2:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2023, 2:56 AM
Herald added a subscriber: yaxunl. · View Herald Transcript
H-G-Hristov requested review of this revision.Feb 26 2023, 2:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2023, 2:56 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Updated papers

H-G-Hristov abandoned this revision.Feb 26 2023, 3:27 AM

Implemented P2711R1 for existing views

  • Fixed placement
  • Unit tests
H-G-Hristov edited the summary of this revision. (Show Details)Feb 26 2023, 5:04 AM
H-G-Hristov retitled this revision from [libc++][spaceship] P2711R1 Making Multi-Param Constructors Of views explicit to [libc++][ranges] P2711R1 Making Multi-Param Constructors Of views explicit.Feb 26 2023, 5:09 AM
H-G-Hristov edited the summary of this revision. (Show Details)Feb 26 2023, 8:05 AM
H-G-Hristov edited the summary of this revision. (Show Details)Mar 1 2023, 9:25 AM
philnik requested changes to this revision.Mar 1 2023, 9:38 AM
philnik added a subscriber: philnik.

With added tests this should be good to go.

libcxx/docs/Status/Cxx2bPapers.csv
111 ↗(On Diff #500549)

Please add a note that join_with_view isn't yet implemented, so we know what's missing to complete it.

libcxx/include/__ranges/drop_view.h
72

You are missing a test for this constructor.

libcxx/include/__ranges/iota_view.h
314

You are missing tests for these constructors.

libcxx/include/__ranges/lazy_split_view.h
81

You are missing tests for these constructors.

libcxx/include/__ranges/split_view.h
78

You are missing tests for these constructors.

libcxx/include/__ranges/take_view.h
67

You are missing a test for this constructor.

libcxx/test/std/ranges/range.adaptors/range.filter/ctor.view_pred.pass.cpp
46 ↗(On Diff #500549)

We normally don't use aliases to make it easier to see what is tested. For a single instance it's also not really worth it.

90 ↗(On Diff #500549)

We generally have SFINAE tests at the top of the file, so let's move them there.

This revision now requires changes to proceed.Mar 1 2023, 9:38 AM
H-G-Hristov retitled this revision from [libc++][ranges] P2711R1 Making Multi-Param Constructors Of views explicit to [libc++][ranges] P2711R1 Making multi-param constructors of views explicit.Mar 21 2023, 9:34 AM
H-G-Hristov edited the summary of this revision. (Show Details)
H-G-Hristov edited the summary of this revision. (Show Details)Mar 21 2023, 9:34 AM
  • Updated implementation and added tests
H-G-Hristov marked 8 inline comments as done.Mar 21 2023, 9:53 AM

@philnik Thank you very much for the review! I hope I addressed all comments as requested.

philnik accepted this revision.Mar 21 2023, 10:27 AM

LGTM % nits.

libcxx/include/__config
874–878 ↗(On Diff #507025)

Please move this up to the _LIBCPP_EXPLICIT_SINCE_CXX14.

libcxx/include/__ranges/filter_view.h
68–69
This revision is now accepted and ready to land.Mar 21 2023, 10:27 AM
  • Addressed review comments and tried to fix CI
H-G-Hristov marked 2 inline comments as done.Mar 21 2023, 12:10 PM
H-G-Hristov added inline comments.
libcxx/include/__ranges/filter_view.h
68–69

It looks like that the clang-format auto-fixer has it own oppinion.

Note I just skimmed over the patch out of curiosity and I didn't do a real review. I noticed some issues.

libcxx/docs/Status/Cxx2b.rst
46 ↗(On Diff #507078)
libcxx/test/std/ranges/range.adaptors/range.drop.while/ctor.view.pass.cpp
38 ↗(On Diff #507078)

We shouldn't use _LIBCPP_foo macros in our test suite. Other vendors like MSVC STL and libstdc++ use our tests too. They don't define _LIBCPP_foo macros.

Note this change requires #include "test_macros.h" too.

The same applies to the other tests for explicit.

H-G-Hristov marked an inline comment as done.Mar 21 2023, 2:06 PM

Note I just skimmed over the patch out of curiosity and I didn't do a real review. I noticed some issues.

Thanks for the remarks. I'll fix those.

Addressed comments

H-G-Hristov marked 2 inline comments as done.Mar 22 2023, 11:56 AM

@philnik @Mordante

Thank you for the reviews!

I'd like to confirm what would be appropriate course of action on my side at this point in general:

  1. I believe I addressed all remarks (IMO).
  2. I fixed all related CI errors but there is still one unrelated (IMO).
  3. I got LGTM but I still made changes after that.

In the context of the above at this point:

  1. Shall I request a re-review explicitly.
  2. Shall I wait for another review.
  3. Shall I decide on my own if I am good to land the patch or wait for 1) or 2)
philnik accepted this revision.Mar 23 2023, 2:10 AM

@philnik @Mordante

Thank you for the reviews!

I'd like to confirm what would be appropriate course of action on my side at this point in general:

  1. I believe I addressed all remarks (IMO).
  2. I fixed all related CI errors but there is still one unrelated (IMO).
  3. I got LGTM but I still made changes after that.

In the context of the above at this point:

  1. Shall I request a re-review explicitly.
  2. Shall I wait for another review.
  3. Shall I decide on my own if I am good to land the patch or wait for 1) or 2)

It's generally a judgement call on your side. It depends on how much the patch changed. In this case IMO it would be OK for you to just land the patch, but it's also OK if you ask again.

This revision was landed with ongoing or failed builds.Mar 23 2023, 10:30 AM
This revision was automatically updated to reflect the committed changes.

@philnik @Mordante

Thank you for the reviews!

I'd like to confirm what would be appropriate course of action on my side at this point in general:

  1. I believe I addressed all remarks (IMO).
  2. I fixed all related CI errors but there is still one unrelated (IMO).
  3. I got LGTM but I still made changes after that.

In the context of the above at this point:

  1. Shall I request a re-review explicitly.
  2. Shall I wait for another review.
  3. Shall I decide on my own if I am good to land the patch or wait for 1) or 2)

It's generally a judgement call on your side. It depends on how much the patch changed. In this case IMO it would be OK for you to just land the patch, but it's also OK if you ask again.

+1 but in general you're free to commit, unless you make "large" changes to fix the CI. That's a judgement call.

Note if I (or a different reviewer) disagrees with the original approval they can request change (the red circle with a cross next to the review), which means a new approval is required. In this case I deemed the changes small enough that I trust you to address them correctly and it doesn't need another review.

I took a look at the changes you made and I was right to trust you :-)