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

[zoecarver]

Summary

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

[cjdb]

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?

[zoecarver]

libcxx/include/__ranges/data.h

57

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

[zoecarver]

• Addresses Chris's comments.

[ldionne]

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]

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]

• Apply Louis' comments

[zoecarver]

Friendly ping @cjdb and @Quuxplusone.

[cjdb]

libcxx/include/__ranges/data.h

36

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

43

__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 :-)

[zoecarver]

• Apply Chris' comments

[cjdb]

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]

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

[zoecarver]

Fix comment empty -> data.

[zoecarver]

Friendly ping @cjdb and @Quuxplusone.

[cjdb]

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.

[zoecarver]

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]

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]

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.

libcxx/include/CMakeLists.txt

set(files
   __memory/temporary_buffer.h
   __memory/uninitialized_algorithms.h
   __memory/unique_ptr.h
   __mutex_base
   __node_handle
   __nullptr
   __ranges/access.h
   __ranges/concepts.h
+  __ranges/data.h
   __ranges/enable_borrowed_range.h
   __ranges/view.h
   __ranges/size.h
   __split_buffer
   __std_stream
   __string
   __threading_support
   __tree

libcxx/include/__ranges/data.h

+// -*- C++ -*-
+//===------------------------ __ranges/data.h ------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#ifndef _LIBCPP___RANGES_DATA_H
+#define _LIBCPP___RANGES_DATA_H
+
+#include <__config>
+
+#include <concepts>
+#include <__iterator/concepts.h>
+#include <__iterator/iterator_traits.h>
+#include <__ranges/access.h>
+#include <type_traits>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#pragma GCC system_header
+#endif
+
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+
+// clang-format off
+namespace ranges {
+// [range.prim.data]
+namespace __data {
+  template <class _Tp>
+  concept __not_invalid_rvalue = __can_borrow<_Tp> || !is_rvalue_reference_v<_Tp>;
+
+  template <class _Tp>
+  concept __not_incomplete_array =
+    !is_array_v<_Tp> || requires { sizeof(remove_all_extents_t<_Tp>); };
+
+  template <class _Tp>
+  concept __wellformed = __not_invalid_rvalue<_Tp> && __not_incomplete_array<_Tp>;
+
+  template <class _Tp>
+  concept __ptr_to_object = is_pointer_v<_Tp> && is_object_v<remove_pointer_t<_Tp>>;
+
+  template <class _Tp>
+  concept __member_data =
+    requires(_Tp&& __t) {
+      { _VSTD::forward<_Tp>(__t) } -> __not_invalid_rvalue;
+      { __t.data() } -> __ptr_to_object;
+    };
+
+  template <class _Tp>
+  concept __ranges_begin =
+    !__member_data<_Tp> &&
+    requires(_Tp&& __t) {
+      { _VSTD::forward<_Tp>(__t) } -> __not_invalid_rvalue;
+      { ranges::begin(_VSTD::forward<_Tp>(__t)) } -> contiguous_iterator;
+    };
+
+  struct __fn {
+    template <__member_data _Tp>
+      requires __wellformed<_Tp>
+    constexpr __ptr_to_object auto operator()(_Tp&& __t) const
+        noexcept(noexcept(__t.data())) {
+      return __t.data();
+    }
+
+    template<__ranges_begin _Tp>
+      requires __wellformed<_Tp>
+    constexpr __ptr_to_object auto operator()(_Tp&& __t) const
+        noexcept(noexcept(_VSTD::to_address(ranges::begin(_VSTD::forward<_Tp>(__t))))) {
+      return _VSTD::to_address(ranges::begin(_VSTD::forward<_Tp>(__t)));
+    }
+  };
+} // end namespace __data
+
+inline namespace __cpo {
+  inline constexpr const auto data = __data::__fn{};
+} // namespace __cpo
+} // namespace ranges
+
+// clang-format off
+
+#endif // !defined(_LIBCPP_HAS_NO_RANGES)
+
+_LIBCPP_END_NAMESPACE_STD
+
+_LIBCPP_POP_MACROS
+
+#endif // _LIBCPP___RANGES_DATA_H

libcxx/include/ranges

template <class _Tp>
     concept common_range = see below;
 }
 
 */
 
 #include <__config>
 #include <__ranges/access.h>
 #include <__ranges/concepts.h>
+#include <__ranges/data.h>
 #include <__ranges/enable_borrowed_range.h>
 #include <__ranges/view.h>
 #include <__ranges/size.h>
 #include <compare>          // Required by the standard.
 #include <initializer_list> // Required by the standard.
 #include <iterator>         // Required by the standard.
 #include <type_traits>
 #include <version>

libcxx/test/std/ranges/range.access/range.prim/data.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: gcc-10
+
+// std::ranges::data
+
+#include <ranges>
+
+#include <cassert>
+#include "test_macros.h"
+#include "test_iterators.h"
+
+using RangeDataT = decltype(std::ranges::data);
+
+static int globalBuff[2];
+
+struct Incomplete;
+
+static_assert(!std::is_invocable_v<RangeDataT, Incomplete[]>);
+static_assert(!std::is_invocable_v<RangeDataT, Incomplete[2]>);
+static_assert(!std::is_invocable_v<RangeDataT, Incomplete[2][2]>);
+static_assert(!std::is_invocable_v<RangeDataT, int [1]>);
+static_assert(!std::is_invocable_v<RangeDataT, int (&&)[1]>);
+static_assert( std::is_invocable_v<RangeDataT, int (&)[1]>);
+
+struct DataMember {
+  int x;
+  constexpr const int *data() const { return &x; }
+};
+
+static_assert( std::is_invocable_v<RangeDataT, DataMember &>);
+static_assert(!std::is_invocable_v<RangeDataT, DataMember &&>);
+
+struct VoidDataMember {
+  void *data() const;
+};
+static_assert(!std::is_invocable_v<RangeDataT, VoidDataMember const&>);
+
+struct Empty { };
+struct EmptyDataMember {
+  Empty data() const;
+};
+
+static_assert(!std::is_invocable_v<RangeDataT, EmptyDataMember const&>);
+
+struct EmptyPtrDataMember {
+  Empty x;
+  constexpr const Empty *data() const { return &x; }
+};
+
+struct PtrConvertible {
+  operator int*() const;
+};
+struct PtrConvertibleDataMember {
+  PtrConvertible data() const;
+};
+
+static_assert(!std::is_invocable_v<RangeDataT, PtrConvertibleDataMember const&>);
+
+struct NonConstDataMember {
+  int x;
+  constexpr int *data() { return &x; }
+};
+
+struct EnabledBorrowingDataMember {
+  constexpr int *data() { return &globalBuff[0]; }
+};
+
+template<>
+inline constexpr bool std::ranges::enable_borrowed_range<EnabledBorrowingDataMember> = true;
+
+struct DataMemberAndBegin {
+  int x;
+  constexpr const int *data() const { return &x; }
+  constexpr const int *begin() const { return &x; }
+};
+
+constexpr bool testDataMember() {
+  DataMember a;
+  assert(std::ranges::data(a) == &a.x);
+
+  NonConstDataMember b;
+  assert(std::ranges::data(b) == &b.x);
+
+  EnabledBorrowingDataMember c;
+  assert(std::ranges::data(std::move(c)) == &globalBuff[0]);
+
+  DataMemberAndBegin d;
+  assert(std::ranges::data(d) == &d.x);
+
+  return true;
+}
+
+using ContiguousIter = contiguous_iterator<const int*>;
+
+struct BeginMemberContiguousIterator {
+  int buff[8];
+
+  constexpr ContiguousIter begin() const { return ContiguousIter(buff); }
+};
+
+static_assert( std::is_invocable_v<RangeDataT, BeginMemberContiguousIterator &>);
+static_assert(!std::is_invocable_v<RangeDataT, BeginMemberContiguousIterator &&>);
+
+struct BeginMemberRandomAccess {
+  int buff[8];
+
+  random_access_iterator<const int*> begin() const;
+};
+
+static_assert(!std::is_invocable_v<RangeDataT, BeginMemberRandomAccess>);
+static_assert(!std::is_invocable_v<RangeDataT, BeginMemberRandomAccess const>);
+static_assert(!std::is_invocable_v<RangeDataT, BeginMemberRandomAccess const&>);
+
+struct BeginFriendContiguousIterator {
+  int buff[8];
+
+  constexpr friend ContiguousIter begin(const BeginFriendContiguousIterator &iter) {
+    return ContiguousIter(iter.buff);
+  }
+};
+
+struct BeginFriendRandomAccess {
+  friend random_access_iterator<const int*> begin(const BeginFriendRandomAccess iter);
+};
+
+static_assert(!std::is_invocable_v<RangeDataT, BeginFriendRandomAccess>);
+static_assert(!std::is_invocable_v<RangeDataT, BeginFriendRandomAccess const>);
+static_assert(!std::is_invocable_v<RangeDataT, BeginFriendRandomAccess const&>);
+
+struct BeginMemberRvalue {
+  int buff[8];
+
+  ContiguousIter begin() &&;
+};
+
+static_assert(!std::is_invocable_v<RangeDataT, BeginMemberRvalue&&>);
+static_assert(!std::is_invocable_v<RangeDataT, BeginMemberRvalue const&>);
+
+struct BeginMemberBorrowingEnabled {
+  constexpr contiguous_iterator<int*> begin() { return contiguous_iterator<int*>{&globalBuff[1]}; }
+};
+
+template<>
+inline constexpr bool std::ranges::enable_borrowed_range<BeginMemberBorrowingEnabled> = true;
+
+constexpr bool testViaRangesBegin() {
+  int arr[2];
+  assert(std::ranges::data(arr) == arr + 0);
+
+  BeginMemberContiguousIterator a;
+  assert(std::ranges::data(a) == a.buff);
+
+  const BeginFriendContiguousIterator b {};
+  assert(std::ranges::data(b) == b.buff);
+
+  BeginMemberBorrowingEnabled c;
+  assert(std::ranges::data(std::move(c)) == &globalBuff[1]);
+
+  return true;
+}
+
+int main(int, char**) {
+  testDataMember();
+  static_assert(testDataMember());
+
+  testViaRangesBegin();
+  static_assert(testViaRangesBegin());
+
+  return 0;
+}