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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Sat May 8 08:48:50 PDT 2021

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.

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list