[libcxx-commits] [PATCH] D101476: [libcxx][ranges] Add ranges::data CPO.

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb requested changes to this revision.
cjdb added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/data.h:36
+  template <class _Tp>
+  concept __not_invalid_rvalue = __can_borrow<_Tp> || !is_rvalue_reference_v<_Tp>;
+
----------------
I'm not sure what more this bit is checking.


================
Comment at: libcxx/include/__ranges/data.h:43
+  template <class _Tp>
+  concept __wellformed = __not_invalid_rvalue<_Tp> && __not_incomplete_array<_Tp>;
+
----------------
`__wellformed` is really non-descriptive. Can we get a better name please?


================
Comment at: libcxx/include/__ranges/data.h:62-66
+    template <__member_data _Tp>
+    [[nodiscard]] constexpr __ptr_to_object auto operator()(_Tp&& __t) const
+        noexcept(noexcept(__t.data())) {
+      return __t.data();
+    }
----------------
zoecarver wrote:
> ldionne wrote:
> > 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.
> 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 :)
> 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).


================
Comment at: libcxx/include/__ranges/data.h:62-66
+    template <__member_data _Tp>
+    [[nodiscard]] constexpr __ptr_to_object auto operator()(_Tp&& __t) const
+        noexcept(noexcept(__t.data())) {
+      return __t.data();
+    }
----------------
cjdb wrote:
> zoecarver wrote:
> > ldionne wrote:
> > > 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.
> > 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 :)
> > 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).
> 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 :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101476/new/

https://reviews.llvm.org/D101476