Page MenuHomePhabricator

[libcxx][ranges] Add ranges::data CPO.
ClosedPublic

Authored by zoecarver on Apr 28 2021, 11:03 AM.

Details

Reviewers
ldionne
EricWF
Quuxplusone
cjdb
Group Reviewers
Restricted Project
Commits
rG9db55b314b5b: [libcxx][ranges] Add ranges::data CPO.
Summary

This is the second to last one! Based on D101396. Depends on D100255. Refs D101079 and D101193.

Diff Detail

Event Timeline

zoecarver created this revision.Apr 28 2021, 11:03 AM
zoecarver requested review of this revision.Apr 28 2021, 11:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2021, 11:03 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb requested changes to this revision.Apr 28 2021, 8:46 PM

Please also add checks for the noexcept specifiers.

libcxx/include/__ranges/data.h
35–39

Isn't this __can_borrow from D100255?

57

I think this is missing !__member_data<_Tp>. This suggests a test where r.data() and ranges::begin(r) are both valid needs to be added.

I'm also unsure as to why it's called __unqualified_begin?

This revision now requires changes to proceed.Apr 28 2021, 8:46 PM
zoecarver added inline comments.May 3 2021, 1:39 PM
libcxx/include/__ranges/data.h
57

Good catch. I'll name it __ranges_begin or something.

zoecarver updated this revision to Diff 342538.May 3 2021, 1:51 PM
  • Addresses Chris's comments.

Overall LGTM. Leaving some comments but not requesting changes so you don't feel blocked to submit since I'm going OOO for a bit.

libcxx/include/__ranges/data.h
14–18

Those should be sorted.

63–67

I think it would be clearer to refactor the code like this:

First, don't mention __not_incomplete_array or __not_invalid_rvalue in the __ranges_begin and __member_data concepts above.

Second, create a concept like this (and de-negate the two helper concepts):

template <class _Tp>
concept __illformed = __invalid_rvalue<_Tp> || __incomplete_array<_Tp>;

Finally, move the invalidity checks to the operator() themselves, like this:

template <__member_data _Tp>
  requires !__illformed<_Tp&&>
constexpr __ptr_to_object auto operator()(_Tp&& __t) const { ... }

template<__ranges_begin _Tp>
  requires !__illformed<_Tp&&>
constexpr __ptr_to_object auto operator()(_Tp&& __t) const { ... }

This is just a suggestion, but I think it would be clearer than repeating the ill-formed requirements in the __member_data nad __ranges_begin concepts. It would also make those concepts do exactly one thing, i.e. detect whether to use the member data or not.

Also, IMO this is abusive use of [[nodiscard]]. We had an offline discussion with Chris about where to put it and where not to, and basically what I said is that we should only put the attribute in places that are an obvious bug if the user doesn't use the return value. That's not the case here. Feel free to keep it or remove it, in all cases we can have a discussion about this and settle on a policy when I come back from vacation.

zoecarver added inline comments.May 10 2021, 2:11 PM
libcxx/include/__ranges/data.h
63–67

I updated this patch to apply those suggestions. The only thing I changed: making the concepts "positive" so that they are __wellformed and __not_incomplete_array. The reason for this is two fold: first it allows us to write requires concept<T> and not requires (!concept<T>) which is easier to read IMO (both because you don't have the extra parens and band, and because you don't have to negate the concept mentally). Second, it allows us to do something like this:

{ _VSTD::forward<_Tp>(__t) } -> __not_invalid_rvalue;

Which is quite nice :)

zoecarver updated this revision to Diff 344205.May 10 2021, 2:12 PM
  • Apply Louis' comments
cjdb requested changes to this revision.May 10 2021, 3:56 PM
cjdb added inline comments.
libcxx/include/__ranges/data.h
37

I'm not sure what more this bit is checking.

44

__wellformed is really non-descriptive. Can we get a better name please?

63–67

Also, IMO this is abusive use of [[nodiscard]]. We had an offline discussion with Chris about where to put it and where not to, and basically what I said is that we should only put the attribute in places that are an obvious bug if the user doesn't use the return value. That's not the case here. Feel free to keep it or remove it, in all cases we can have a discussion about this and settle on a policy when I come back from vacation.

I personally think it's a bug for someone to have exactly ranges::data(r); somewhere in their code. While I might disagree with the ranges::advance(i, n, s) case, I appreciate the argument that the salient feature is a side effect. That isn't the case here: the salient feature of ranges::data is its return value (even if it defers to something with side effects). As such, I consider [[nodiscard]] to be an important attribute for ranges::data (and all CPOs, for the same reason).

63–67

I updated this patch to apply those suggestions. The only thing I changed: making the concepts "positive" so that they are __wellformed and __not_incomplete_array. The reason for this is two fold: first it allows us to write requires concept<T> and not requires (!concept<T>) which is easier to read IMO (both because you don't have the extra parens and band, and because you don't have to negate the concept mentally). Second, it allows us to do something like this:

{ _VSTD::forward<_Tp>(__t) } -> __not_invalid_rvalue;

Which is quite nice :)

If you invert the concept definitions, then you should be able to rename them. For example __not_invalid_rvalue would become __valid_rvalue, which is easier to read again :-)

This revision now requires changes to proceed.May 10 2021, 3:56 PM
  • Apply Chris' comments
cjdb added inline comments.May 11 2021, 11:39 AM
libcxx/include/__ranges/data.h
41

Similarly to the CPOs in __ranges/access.h, we want arrays of incomplete types to be SFINAE-unfriendly.

zoecarver updated this revision to Diff 345578.May 14 2021, 3:43 PM

Make unbound, incomplete, lvalue reference array types have a hard error.

zoecarver updated this revision to Diff 345579.May 14 2021, 3:44 PM

Fix comment empty -> data.

cjdb added inline comments.May 16 2021, 9:36 PM
libcxx/include/__ranges/data.h
47

This is easily misread as ranges::begin (I have on several occasions). Perhaps __can_invoke_ranges_begin?

51

Please use iterator_t instead of redefining it.

63–67

Please don't merge until this is resolved.

I'll apply the other two things before I land it.

libcxx/include/__ranges/data.h
63–67

Did Louis' recent mailing list message resolve this?

ldionne accepted this revision.May 21 2021, 10:26 AM
ldionne added inline comments.
libcxx/include/__ranges/data.h
63–67

We agreed yesterday on Discord to not add [[nodiscard]] here until we've settled on a decision, so @zoecarver you're not blocked on this.

ldionne added inline comments.May 21 2021, 10:28 AM
libcxx/include/__ranges/data.h
51

I'm ambivalent on this, since the wording in http://eel.is/c++draft/range.prim.data#2.4 clearly says ranges::begin instead of iterator_t. It might be an oversight in the standard.

@zoecarver I'll let you judge whether you want to keep it the way it is right now or apply Chris' suggestion.

This revision was not accepted when it landed; it landed in state Needs Review.May 21 2021, 11:10 AM
This revision was automatically updated to reflect the committed changes.