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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

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.



================
Comment at: libcxx/include/__ranges/data.h:13-17
+#include <concepts>
+#include <__iterator/iterator_traits.h>
+#include <__iterator/concepts.h>
+#include <__ranges/begin.h>
+#include <type_traits>
----------------
Those should be sorted.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101476