[libcxx] Implement view.interface.

[zoecarver]

Summary

This will unblock work on ranges::view. Based on D101396.

Refs http://eel.is/c++draft/view.interface.

[zoecarver]

I wanted to get this review up before Monday so that other folks can start on their views work. I'm planning a few changes (mainly to add some tests) so this isn't quite ready for review yet.

libcxx/include/__ranges/view_interface.h

38

I will remove this once the patch lands and I rebase.

131

Add a message here.

150

Review note: this is used until ranges::prev is implemented.

[zoecarver]

• Fix spacing.

[cjdb]

A good start! I'd appreciate some libcxx tests confirming that _LIBCPP_ASSERT fires when a view is empty, and some noexcept tests.

libcxx/include/__ranges/view_interface.h

38

It doesn't look like the concept is used in your patch anyway?

46

Please make this a reference.

60

Oh, this issue. I remember when gcc -fconcepts had this problem back in the day :(

150

Thanks for highlighting this, was about to comment.

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

35

Can we please get a pointer-to-member and one pointer-to-member function too?

49–52

Nit: consider making this a hidden friend.

103

Please either rename or document why this function exists.

112

Nit: fv or fr? (I'd personally go with single-letter names to avoid the mental "what does this letter correspond to?".)

156

I think view_interface::data (the real one) is a function template, so it'll prefer the existing member function. @tcanens how far from the mark am I?

[tcanens]

libcxx/include/__ranges/data.h

58 ↗

(On Diff #342305)

missing !?

[zoecarver]

libcxx/include/__ranges/view_interface.h

46

In this case T = Derived so I don't think it matters. I've added a test where *this is of type move only rvalue.

60

:(

Also, I couldn't do __can_empty inline.

And if you don't have this, it fails in a very confusing way. Spent a lot of time figuring out why view_interface sort of thought _Derived was an incomplete type but could call members on the type. I finally figured it out when I discovered that it only appeared to be an incomplete type when evaluating the constraints. I suspect clang is simply forgetting a record = record->getDefinition() somewhere.

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

49–52

Am I able to define a hidden friend out of line? I don't want to update forward_iterator (unless we agree that would be a good idea).

156

That would make sense, except that it's the child that's getting called, i.e, view_interface::data. Regardless of which one is a template, I thought it would always go for the parent's definition.

[zoecarver]

• Apply review comments and general cleanup.

[cjdb]

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

49–52

Oh, I see, it's not for the above type. My mistake.

156

BTW, the same is true for SFINAE.

[zoecarver]

libcxx/include/__ranges/view_interface.h

118

Remove all the nodiscards except for empty.

[• Quuxplusone]

libcxx/include/__ranges/view_interface.h

118

It would be appropriate to mark them _LIBCPP_NODISCARD_EXT (with tests).

[cjdb]

libcxx/include/__ranges/view_interface.h

118

Remove all the nodiscards except for empty.

Please elaborate.

It would be appropriate to mark them _LIBCPP_NODISCARD_EXT (with tests).

Why is this different to [[nodiscard]] + disabling the warning?

[zoecarver]

libcxx/include/__ranges/view_interface.h

118

Please elaborate.

We've discussed elsewhere, I think libc++ is only going to apply nodiscard to methods where a user might think that a side effect is happening (such as .empty()).

[zoecarver]

• Apply review comments.

[zoecarver]

Rebase on main.

[ldionne]

Generally LGTM, with some comments!

libcxx/include/CMakeLists.txt

43–46

Should be sorted!

libcxx/include/__ranges/view_interface.h

38

Can drop nodiscard.

55

Why is that necessary? Is that the workaround for gcc -fconcepts you're discussing in the comments below?

150

This can be changed now (and also in the noexcept(...) above).

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

12

I don't think this is necessary anymore.

184

Can you elaborate on what this TODO means?

[CaseyCarter]

libcxx/include/__ranges/view_interface.h

49

You're missing that the implicit conversion to of this expression to bool could be potentially throwing. We use something like:

c++
template <class T>
void implicit_convert_to(type_identity_t<T>) noexcept;
/* ... */
  noexcept(noexcept(implicit_convert_to<bool>(ranges::begin(__derived()) == ranges::end(__derived()))))

55

This is necessary to workaround https://bugs.llvm.org/show_bug.cgi?id=44833.

[zoecarver]

libcxx/include/__ranges/view_interface.h

49

Done and added a regression test.

libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp

184

No longer applies, removed.

(IIRC, that assert failed because it was using view_interface's implementation for data.)

[zoecarver]

Apply review comments.

[zoecarver]

• Rebase.
• -- -> prev (again).

[ldionne]

LGTM, but please wait for CI to finish.

(Also, we just spoke offline and you mentioned you needed to update the Status paper and the synopsis - please do it before landing).

libcxx/include/__ranges/view_interface.h

55

Thanks!

libcxx/include/CMakeLists.txt

set(files
   __mutex_base
   __node_handle
   __nullptr
   __ranges/access.h
   __ranges/concepts.h
   __ranges/data.h
   __ranges/empty.h
   __ranges/enable_borrowed_range.h
   __ranges/view.h
   __ranges/size.h
+  __ranges/view_interface.h
   __split_buffer
   __std_stream
   __string
   __threading_support
   __tree
   __tuple
   __undef_macros
   __utility/to_underlying.h
   algorithm

libcxx/include/__ranges/view_interface.h

+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// 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_VIEW_INTERFACE_H
+#define _LIBCPP___RANGES_VIEW_INTERFACE_H
+
+#include <__config>
+#include <__iterator/iterator_traits.h>
+#include <__iterator/concepts.h>
+#include <__ranges/access.h>
+#include <__ranges/empty.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)
+
+namespace ranges {
+
+template<class _Tp>
+concept __can_empty = requires(_Tp __t) { ranges::empty(__t); };
+
+template<class _Derived>
+  requires is_class_v<_Derived> && same_as<_Derived, remove_cv_t<_Derived>>
+class view_interface : public view_base {
+  [[nodiscard]] constexpr _Derived& __derived() noexcept {
+    return static_cast<_Derived&>(*this);
+  }
+
+  [[nodiscard]] constexpr _Derived const& __derived() const noexcept {
+    return static_cast<_Derived const&>(*this);
+  }
+
+public:
+  template<class _D2 = _Derived>
+  [[nodiscard]] constexpr bool empty()
+    noexcept(noexcept(ranges::begin(__derived()) == ranges::end(__derived())))
+    requires forward_range<_D2>
+  {
+    return ranges::begin(__derived()) == ranges::end(__derived());
+  }
+
+  template<class _D2 = _Derived>
+  [[nodiscard]] constexpr bool empty() const
+    noexcept(noexcept(ranges::begin(__derived()) == ranges::end(__derived())))
+    requires forward_range<const _D2>
+  {
+    return ranges::begin(__derived()) == ranges::end(__derived());
+  }
+
+  template<class _D2 = _Derived>
+  constexpr explicit operator bool()
+    noexcept(noexcept(ranges::empty(declval<_D2>())))
+    requires __can_empty<_D2>
+  {
+    return !ranges::empty(__derived());
+  }
+
+  template<class _D2 = _Derived>
+  constexpr explicit operator bool() const
+    noexcept(noexcept(ranges::empty(declval<const _D2>())))
+    requires __can_empty<const _D2>
+  {
+    return !ranges::empty(__derived());
+  }
+
+  template<class _D2 = _Derived>
+  constexpr auto data()
+    noexcept(noexcept(_VSTD::to_address(ranges::begin(__derived()))))
+    requires contiguous_iterator<iterator_t<_D2>>
+  {
+    return _VSTD::to_address(ranges::begin(__derived()));
+  }
+
+  template<class _D2 = _Derived>
+  constexpr auto data() const
+    noexcept(noexcept(_VSTD::to_address(ranges::begin(__derived()))))
+    requires range<const _D2> && contiguous_iterator<iterator_t<const _D2>>
+  {
+    return _VSTD::to_address(ranges::begin(__derived()));
+  }
+
+  template<class _D2 = _Derived>
+  constexpr auto size()
+    noexcept(noexcept(ranges::end(__derived()) - ranges::begin(__derived())))
+    requires forward_range<_D2>
+      && sized_sentinel_for<sentinel_t<_D2>, iterator_t<_D2>>
+  {
+    return ranges::end(__derived()) - ranges::begin(__derived());
+  }
+
+  template<class _D2 = _Derived>
+  constexpr auto size() const
+    noexcept(noexcept(ranges::end(__derived()) - ranges::begin(__derived())))
+    requires forward_range<const _D2>
+      && sized_sentinel_for<sentinel_t<const _D2>, iterator_t<const _D2>>
+  {
+    return ranges::end(__derived()) - ranges::begin(__derived());
+  }
+
+  template<class _D2 = _Derived>
+  constexpr decltype(auto) front()
+    noexcept(noexcept(*ranges::begin(__derived())))
+    requires forward_range<_D2>
+  {
+    _LIBCPP_ASSERT(!empty(),
+        "Precondition `!empty()` not satisfied. `.front()` called on an empty view.");
+    return *ranges::begin(__derived());
+  }
+
+  template<class _D2 = _Derived>
+  constexpr decltype(auto) front() const
+    noexcept(noexcept(*ranges::begin(__derived())))
+    requires forward_range<const _D2>
+  {
+    _LIBCPP_ASSERT(!empty(),
+        "Precondition `!empty()` not satisfied. `.front()` called on an empty view.");
+    return *ranges::begin(__derived());
+  }
+
+  template<class _D2 = _Derived>
+  constexpr decltype(auto) back()
+    noexcept(noexcept(*(--ranges::end(__derived()))))
+    requires bidirectional_range<_D2> && common_range<_D2>
+  {
+    _LIBCPP_ASSERT(!empty(),
+        "Precondition `!empty()` not satisfied. `.back()` called on an empty view.");
+    return *(--ranges::end(__derived()));
+  }
+
+  template<class _D2 = _Derived>
+  constexpr decltype(auto) back() const
+    noexcept(noexcept(*(--ranges::end(__derived()))))
+    requires bidirectional_range<const _D2> && common_range<const _D2>
+  {
+    _LIBCPP_ASSERT(!empty(),
+        "Precondition `!empty()` not satisfied. `.back()` called on an empty view.");
+    return *(--ranges::end(__derived()));
+  }
+
+  template<random_access_range _RARange = _Derived>
+  constexpr decltype(auto) operator[](range_difference_t<_RARange> __index)
+    noexcept(noexcept(ranges::begin(__derived())[__index]))
+  {
+    return ranges::begin(__derived())[__index];
+  }
+
+  template<random_access_range _RARange = const _Derived>
+  constexpr decltype(auto) operator[](range_difference_t<_RARange> __index) const
+    noexcept(noexcept(ranges::begin(__derived())[__index]))
+  {
+    return ranges::begin(__derived())[__index];
+  }
+};
+
+}
+
+#endif // !defined(_LIBCPP_HAS_NO_RANGES)
+
+_LIBCPP_END_NAMESPACE_STD
+
+_LIBCPP_POP_MACROS
+
+#endif // _LIBCPP___RANGES_VIEW_INTERFACE_H

libcxx/include/ranges

 #include <__config>
 #include <__ranges/access.h>
 #include <__ranges/concepts.h>
 #include <__ranges/data.h>
 #include <__ranges/empty.h>
 #include <__ranges/enable_borrowed_range.h>
 #include <__ranges/view.h>
 #include <__ranges/size.h>
+#include <__ranges/view_interface.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>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header

libcxx/test/std/ranges/range.utility/view.interface/view.interface.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
+// XFAIL: msvc && clang
+
+// std::ranges::size
+
+#include <ranges>
+
+#include <cassert>
+#include "test_macros.h"
+#include "test_iterators.h"
+
+namespace ranges = std::ranges;
+
+template<class T>
+concept ValidViewInterfaceType = requires { typename ranges::view_interface<T>; };
+
+struct Empty { };
+
+static_assert(!ValidViewInterfaceType<void>);
+static_assert(!ValidViewInterfaceType<void*>);
+static_assert(!ValidViewInterfaceType<Empty*>);
+static_assert(!ValidViewInterfaceType<Empty const>);
+static_assert(!ValidViewInterfaceType<Empty &>);
+static_assert( ValidViewInterfaceType<Empty>);
+
+static_assert(std::derived_from<ranges::view_interface<Empty>, ranges::view_base>);
+
+using InputIter = cpp20_input_iterator<const int*>;
+
+struct InputRange : ranges::view_interface<InputRange> {
+  int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
+  constexpr InputIter begin() const { return InputIter(buff); }
+  constexpr InputIter end() const { return InputIter(buff + 8); }
+};
+
+struct NotSizedSentinel {
+  using I = int*;
+  using value_type = std::iter_value_t<I>;
+  using difference_type = std::iter_difference_t<I>;
+  using iterator_concept = std::forward_iterator_tag;
+
+  NotSizedSentinel() = default;
+  explicit constexpr NotSizedSentinel(I);
+
+  constexpr int &operator*() const { return *value; };
+  NotSizedSentinel& operator++();
+  NotSizedSentinel operator++(int);
+  bool operator==(NotSizedSentinel const&) const;
+
+  int *value;
+};
+static_assert(std::forward_iterator<NotSizedSentinel>);
+
+using ForwardIter = forward_iterator<int*>;
+
+// So that we conform to sized_sentinel_for.
+constexpr std::ptrdiff_t operator-(const ForwardIter& x, const ForwardIter& y) {
+    return x.base() - y.base();
+}
+
+struct ForwardRange : ranges::view_interface<ForwardRange> {
+  int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
+  constexpr ForwardIter begin() const { return ForwardIter(const_cast<int*>(buff)); }
+  constexpr ForwardIter end() const { return ForwardIter(const_cast<int*>(buff) + 8); }
+};
+
+struct MoveOnlyForwardRange : ranges::view_interface<MoveOnlyForwardRange> {
+  int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
+  MoveOnlyForwardRange(MoveOnlyForwardRange const&) = delete;
+  MoveOnlyForwardRange(MoveOnlyForwardRange &&) = default;
+  MoveOnlyForwardRange() = default;
+  constexpr ForwardIter begin() const { return ForwardIter(const_cast<int*>(buff)); }
+  constexpr ForwardIter end() const { return ForwardIter(const_cast<int*>(buff) + 8); }
+};
+
+struct EmptyIsTrue : ranges::view_interface<EmptyIsTrue> {
+  int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
+  constexpr ForwardIter begin() const { return ForwardIter(const_cast<int*>(buff)); }
+  constexpr ForwardIter end() const { return ForwardIter(const_cast<int*>(buff) + 8); }
+  constexpr bool empty() const { return true; }
+};
+
+struct SizeIsTen : ranges::view_interface<SizeIsTen> {
+  int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
+  constexpr ForwardIter begin() const { return ForwardIter(const_cast<int*>(buff)); }
+  constexpr ForwardIter end() const { return ForwardIter(const_cast<int*>(buff) + 8); }
+  constexpr size_t size() const { return 10; }
+};
+
+using RAIter = random_access_iterator<int*>;
+
+struct RARange : ranges::view_interface<RARange> {
+  int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
+  constexpr RAIter begin() const { return RAIter(const_cast<int*>(buff)); }
+  constexpr RAIter end() const { return RAIter(const_cast<int*>(buff) + 8); }
+};
+
+using ContIter = contiguous_iterator<const int*>;
+
+struct ContRange : ranges::view_interface<ContRange> {
+  int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
+  constexpr ContIter begin() const { return ContIter(buff); }
+  constexpr ContIter end() const { return ContIter(buff + 8); }
+};
+
+struct DataIsNull : ranges::view_interface<DataIsNull> {
+  int buff[8] = {0, 1, 2, 3, 4, 5, 6, 7};
+  constexpr ContIter begin() const { return ContIter(buff); }
+  constexpr ContIter end() const { return ContIter(buff + 8); }
+  constexpr const int *data() const { return nullptr; }
+};
+
+template<class T>
+concept EmptyInvocable = requires (T const& obj) { obj.empty(); };
+
+template<class T>
+concept BoolOpInvocable = requires (T const& obj) { bool(obj); };
+
+constexpr bool testEmpty() {
+  static_assert(!EmptyInvocable<InputRange>);
+  static_assert( EmptyInvocable<ForwardRange>);
+
+  static_assert(!BoolOpInvocable<InputRange>);
+  static_assert( BoolOpInvocable<ForwardRange>);
+
+  ForwardRange forwardRange;
+  assert(!forwardRange.empty());
+  assert(!static_cast<ForwardRange const&>(forwardRange).empty());
+
+  assert(forwardRange);
+  assert(static_cast<ForwardRange const&>(forwardRange));
+
+  assert(!ranges::empty(forwardRange));
+  assert(!ranges::empty(static_cast<ForwardRange const&>(forwardRange)));
+
+  EmptyIsTrue emptyTrue;
+  assert(emptyTrue.empty());
+  assert(static_cast<EmptyIsTrue const&>(emptyTrue).empty());
+  assert(!emptyTrue.ranges::view_interface<EmptyIsTrue>::empty());
+
+  assert(!emptyTrue);
+  assert(!static_cast<EmptyIsTrue const&>(emptyTrue));
+  assert(!emptyTrue.ranges::view_interface<EmptyIsTrue>::operator bool());
+
+  assert(ranges::empty(emptyTrue));
+  assert(ranges::empty(static_cast<EmptyIsTrue const&>(emptyTrue)));
+
+  // Try calling empty on an rvalue.
+  MoveOnlyForwardRange moveOnly;
+  assert(!std::move(moveOnly).empty());
+
+  return true;
+}
+
+template<class T>
+concept DataInvocable = requires (T const& obj) { obj.data(); };
+
+constexpr bool testData() {
+  static_assert(!DataInvocable<ForwardRange>);
+  static_assert( DataInvocable<ContRange>);
+
+  ContRange contiguous;
+  assert(contiguous.data() == contiguous.buff);
+  assert(static_cast<ContRange const&>(contiguous).data() == contiguous.buff);
+
+  assert(ranges::data(contiguous) == contiguous.buff);
+  assert(ranges::data(static_cast<ContRange const&>(contiguous)) == contiguous.buff);
+
+  DataIsNull dataNull;
+  assert(dataNull.data() == nullptr);
+  assert(static_cast<DataIsNull const&>(dataNull).data() == nullptr);
+  assert(dataNull.ranges::view_interface<DataIsNull>::data() == dataNull.buff);
+
+// TODO: why is the child member picked?
+//   assert(ranges::data(dataNull) == nullptr);
+//   assert(ranges::data(static_cast<DataIsNull const&>(dataNull)) == nullptr);
+
+  return true;
+}
+
+template<class T>
+concept SizeInvocable = requires (T const& obj) { obj.size(); };
+
+constexpr bool testSize() {
+  static_assert(!SizeInvocable<InputRange>);
+  static_assert(!SizeInvocable<NotSizedSentinel>);
+  static_assert( SizeInvocable<ForwardRange>);
+
+  ForwardRange forwardRange;
+  assert(forwardRange.size() == 8);
+  assert(static_cast<ForwardRange const&>(forwardRange).size() == 8);
+
+  assert(ranges::size(forwardRange) == 8);
+  assert(ranges::size(static_cast<ForwardRange const&>(forwardRange)) == 8);
+
+  SizeIsTen sizeTen;
+  assert(sizeTen.size() == 10);
+  assert(static_cast<SizeIsTen const&>(sizeTen).size() == 10);
+  assert(sizeTen.ranges::view_interface<SizeIsTen>::size() == 8);
+
+  assert(ranges::size(sizeTen) == 10);
+  assert(ranges::size(static_cast<SizeIsTen const&>(sizeTen)) == 10);
+
+  return true;
+}
+
+template<class T>
+concept SubscriptInvocable = requires (T const& obj, size_t n) { obj[n]; };
+
+constexpr bool testSubscript() {
+  static_assert(!SubscriptInvocable<ForwardRange>);
+  static_assert( SubscriptInvocable<RARange>);
+
+  RARange randomAccess;
+  assert(randomAccess[2] == 2);
+  assert(static_cast<RARange const&>(randomAccess)[2] == 2);
+  randomAccess[2] = 3;
+  assert(randomAccess[2] == 3);
+
+  return true;
+}
+
+template<class T>
+concept FrontInvocable = requires (T const& obj) { obj.front(); };
+
+template<class T>
+concept BackInvocable = requires (T const& obj) { obj.back(); };
+
+constexpr bool testFrontBack() {
+  static_assert(!FrontInvocable<InputRange>);
+  static_assert( FrontInvocable<ForwardRange>);
+  static_assert(!BackInvocable<ForwardRange>);
+  static_assert( BackInvocable<RARange>);
+
+  ForwardRange forwardRange;
+  assert(forwardRange.front() == 0);
+  assert(static_cast<ForwardRange const&>(forwardRange).front() == 0);
+  forwardRange.front() = 2;
+  assert(forwardRange.front() == 2);
+
+  RARange randomAccess;
+  assert(randomAccess.front() == 0);
+  assert(static_cast<RARange const&>(randomAccess).front() == 0);
+  randomAccess.front() = 2;
+  assert(randomAccess.front() == 2);
+
+  assert(randomAccess.back() == 7);
+  assert(static_cast<RARange const&>(randomAccess).back() == 7);
+  randomAccess.back() = 2;
+  assert(randomAccess.back() == 2);
+
+  return true;
+}
+
+int main(int, char**) {
+  testEmpty();
+  static_assert(testEmpty());
+
+  testData();
+  static_assert(testData());
+
+  testSize();
+  static_assert(testSize());
+
+  testSubscript();
+  static_assert(testSubscript());
+
+  testFrontBack();
+  static_assert(testFrontBack());
+
+  return 0;
+}