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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
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();
+    }
----------------
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 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101476