This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][ranges] Add class ref_view.
ClosedPublic

Authored by zoecarver on May 6 2021, 1:20 PM.

Details

Diff Detail

Event Timeline

zoecarver created this revision.May 6 2021, 1:20 PM
zoecarver requested review of this revision.May 6 2021, 1:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2021, 1:20 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zoecarver planned changes to this revision.May 6 2021, 4:39 PM
  • Fix and add tests.

This is also now ready for review.

Quuxplusone requested changes to this revision.May 11 2021, 12:12 PM
Quuxplusone added inline comments.
libcxx/include/CMakeLists.txt
45 ↗(On Diff #344514)

Please alphabetize.

libcxx/include/__ranges/ref_view.h
14–15

Please alphabetize.

40–43

The sized_range concept should go in the same header that defines ranges::size.

I'm fairly confident that concept borrowed_range doesn't belong in ref_view.h either, but I have no particular suggestion of where it should go. It's also a duplicate of concept __can_borrow from __ranges/access.h; please figure something out.

76

Nit: For readability, I'd like to see the requires-clauses on line 75 and 71 line-broken and indented the same way you've done on line 68.

85

Nit: I'd prefer to see all of these noisy // clang-format {off,on} comments eliminated. (I'm planning to do that once this code stabilizes, anyway. Just complainin'. It'd make all your PRs four lines shorter!)

libcxx/include/ranges
86 ↗(On Diff #344514)

Please alphabetize.

libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp
22

In @cjdb's tests he's been using namespace stdr = std::ranges, which has two advantages: shortness and greppability. I think you should make the switch. (My personal preference is namespace rg, but we've already got stdr in the codebase, and anyway it's easy to switch from anything to anything, as long as it's not namespace ranges.)

31–34

friend constexpr, not constexpr friend — but also, why are these friends instead of member begin/end? Making them members would be a lot less typing.

44

But ValidRefView<int[4]> would be OK, right?

83–84

Isn't this the sort of thing that the reversed synthesized candidates are supposed to handle for us?
Orthogonally: hidden friend plz... or actually, wait, what? You're adding comparison operators to cpp20_input_iterator itself? That doesn't seem right. You should just make Cpp20InputRange::end() return a value of type cpp20_input_iterator::sentinel.

106

Throughout: I'd like to see these tests use stdr::ref_view<Range> view = range; instead of direct-curly-brace-initialization.

libcxx/test/support/test_iterators.h
645–648 ↗(On Diff #344514)

Defaulted SMFs are constexpr (and noexcept) by default. You don't need this diff.

657–666 ↗(On Diff #344514)

This looks good. While you're in the vicinity, I recommend moving cpp20_input_iterator up next to the other test iterators (namely, next to cpp17_input_iterator), and making it look like them:

  • remove the C++20 iter_value_t dependency, just use iterator_traits like they do
  • remove the [[nodiscard]] cruft
  • add the missing base(I) free function
  • add the missing deleted operator,
  • ifdef it to C++14-and-later since its definition doesn't depend on anything from '17 or '20 (except iter_value_t which we can remove)
  • add a nested type struct sentinel {} which is comparable to cpp20_input_iterator<T> (via hidden friends)

Ping me if you want me to just do this.

This revision now requires changes to proceed.May 11 2021, 12:12 PM
zoecarver added inline comments.May 13 2021, 9:27 AM
libcxx/include/__ranges/ref_view.h
40–43

I posted this in another thread too, but I did a bad job documenting these concepts. They will both be implemented in their own patches at some point soon (I was hoping for today... but that didn't happen).

I didn't realize they were needed until too late so I just copied them in; they're not part of this review.

libcxx/test/support/test_iterators.h
645–648 ↗(On Diff #344514)

Fair enough, will remove. Semi-related: from the synopsis of ref_view:

constexpr ref_view() noexcept = default;
657–666 ↗(On Diff #344514)

I'll do this in another PR.

zoecarver updated this revision to Diff 345183.May 13 2021, 9:28 AM

Apply Arthur's comments.

Quuxplusone added inline comments.Jun 3 2021, 7:01 AM
libcxx/include/__ranges/ref_view.h
36

__range_

64

Has contiguous_range landed yet? If so, please DO this TODO; otherwise ok, not a blocker.

libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp
67

Pass-by-const-value alert! Remove const.

83–84

This comment relates to the code now on lines 77-78, and it hasn't been addressed yet.

99–106

Naming nit: EmptyIsInvocable, or HasEmpty; but let's avoid any potential confusion with the jargon use of Invocable as a noun. I'd go with HasEmpty, myself; it's shortest.

125

nit: whitespace

ldionne requested changes to this revision.Jun 3 2021, 1:09 PM

Basically LGTM, requesting changes just so it shows up right in the queue since there are a few pending comments to apply.

libcxx/include/__ranges/ref_view.h
41–42

Those can be made private, and I would perhaps rename it to something like __binds_to_lvalue_ref or something like that.

Do you have a better understanding of what this function is used for than I do? IIUC, it's only used to check that __tp in the constructor below would bind to a lvalue, but not to a rvalue. I'm kind of wondering why the Standard is using this obtuse wording -- there must be a good reason?

45

I guess you want to use __not_same_as here. Not a big fan of that helper concept, but oh well.

This revision now requires changes to proceed.Jun 3 2021, 1:09 PM
tcanens added a subscriber: tcanens.Jun 3 2021, 5:28 PM
tcanens added inline comments.
libcxx/include/__ranges/ref_view.h
41–42

This is the LWG2993 dance to avoid having an ICS from rvalues.

zoecarver marked 17 inline comments as done.Jun 4 2021, 12:41 PM

Okay, I think that's everything.

libcxx/include/__ranges/ref_view.h
64

No, another thing I need to do :(

libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp
67

Removed, but it is at least more OK in this situation where the argument is named and used.

99–106

We're not really asking if the member exists, it always does exist. We're asking whether we can call/invoke it. Also, I don't really see how EmptyIsInvocable is any better than EmptyInvocable. (Not done.)

zoecarver updated this revision to Diff 349943.Jun 4 2021, 12:42 PM
zoecarver marked an inline comment as done.

Apply review comments.

Quuxplusone added inline comments.Jun 4 2021, 1:00 PM
libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp
99–106

Also, I don't really see how EmptyIsInvocable is any better than EmptyInvocable.

As I said, it's better in that it avoids (confusion with) the jargon use of "invocable" as a noun. I understand what regular_invocable<R> means, but what is EmptyInvocable<R>? What is an "empty invocable"?

zoecarver updated this revision to Diff 349960.Jun 4 2021, 1:39 PM
  • Diff off correct commit.
  • EmptyInvocable -> EmptyIsInvocable
ldionne accepted this revision.Jun 9 2021, 11:59 AM

LGTM. Please make __binds_to_lvalue_ref private, and we might want to rename it to something like __avoid_ICS_from_rvalue or something like that.

libcxx/include/__ranges/ref_view.h
41–42

Thanks for the context. I must say though, I was unable to construct a case where dropping requires { __binds_to_lvalue_ref(declval<_Tp>()); } would make a difference since we're already checking for convertible_to<_Tp, _Range&>.

In LWG2993, I understand why reference_wrapper used to be broken with the reference_wrapper(T&&) = delete overload, but I fail to see how this applies here. I guess it's not relevant to this review, I'm just curious in case you have time to elaborate.

libcxx/test/std/ranges/range.adaptors/range.ref.view.pass.cpp
2

I just want to say: normally, we'd split this into several files, one per tested thing. But ref_view is almost only its synopsis, so I think this is fine.

libcxx/include/__ranges/ref_view.h
41–42

I believe the only case this catches is when T is ambiguously convertible to both R& and R&&:
https://godbolt.org/z/hT4jraEo7

struct Evil {
    operator std::vector<int>& () const;
    operator std::vector<int>&& () const;
};
static_assert(!std::is_convertible_v<Evil, std::reference_wrapper<std::vector<int>>>);
static_assert(!std::is_convertible_v<Evil, std::ranges::ref_view<std::vector<int>>>);

I don't think there was ever a good reason for reference_wrapper to not-bind-to-rvalues, but it's true that reference_wrapper and ref_view are doing the exact same dance here and for exactly isomorphic reasons. (And that's a good reason to keep the naming convention the same, btw. reference_wrapper calls the helper function __fun, and I think we should do exactly the same here.)

Still LGTM.

libcxx/include/__ranges/ref_view.h
41–42

calls the helper function __fun, and I think we should do exactly the same here

Yeah, I considering that when I wrote my comment and I think the fact that you're suggesting it is enough to sway me. @zoecarver , if you agree, I would go back to the not-very-descriptive-but-consistent-and-close-to-the-standard __fun naming you had initially.

Perhaps you could also add a test like what Arthur suggested?

tcanens added inline comments.Jun 9 2021, 8:26 PM
libcxx/include/__ranges/ref_view.h
41–42

_Range can be const-qualified, and then _Range& can bind to rvalues, which is what this is trying to disallow.

I'm not sure if this actually needs the 2993 do-not-even-have-an-ICS treatment though (as opposed to = delete;). It's pretty straightforward to come up with toy examples where it makes a difference, but realistic examples are harder to come by.

ldionne requested changes to this revision.Jun 10 2021, 10:20 AM

Per offline discussion,

  • Change back to __fun
  • Add a test like Arthur suggested
  • Update for P2325
This revision now requires changes to proceed.Jun 10 2021, 10:20 AM

Apply Louis' feedback.

Remove added empty file.

zoecarver updated this revision to Diff 351261.Jun 10 2021, 2:15 PM
  • Fix indenting.
  • Remove white space
  • Remove no-longer-needed operator==s.
ldionne accepted this revision.Jun 11 2021, 6:00 AM

Pending passing CI, of course.

zoecarver updated this revision to Diff 351474.Jun 11 2021, 9:26 AM

Add ref_view.h to the cmake lists after it was removed during rebase (and not caught locally due to caching).

This revision was not accepted when it landed; it landed in state Needs Review.Jun 11 2021, 11:04 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.