[libcxx-commits] [PATCH] D101737: [libcxx] Implement view.interface.

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb requested changes to this revision.
cjdb added a subscriber: tcanens.
cjdb added a comment.
This revision now requires changes to proceed.

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



================
Comment at: libcxx/include/__ranges/view_interface.h:37
+template <class _Tp>
+concept view =
+  range<_Tp> &&
----------------
zoecarver wrote:
> I will remove this once the patch lands and I rebase. 
It doesn't look like the concept is used in your patch anyway?


================
Comment at: libcxx/include/__ranges/view_interface.h:149
+    _LIBCPP_ASSERT(!empty(), "");
+    return *(--ranges::end(__derived()));
+  }
----------------
zoecarver wrote:
> Review note: this is used until `ranges::prev` is implemented.
Thanks for highlighting this, was about to comment.


================
Comment at: libcxx/include/__ranges/view_interface.h:45
+template<class _Tp>
+concept __can_empty = requires(_Tp __t) { ranges::empty(__t); };
+
----------------
Please make this a reference.


================
Comment at: libcxx/include/__ranges/view_interface.h:59
+public:
+  template<class _D2 = _Derived>
+  [[nodiscard]] constexpr bool empty()
----------------
Oh, //this// issue. I remember when `gcc -fconcepts` had this problem back in the day :(


================
Comment at: libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp:34
+static_assert(!ValidViewInterfaceType<Empty &>);
+static_assert( ValidViewInterfaceType<Empty>);
+
----------------
Can we please get a pointer-to-member and one pointer-to-member function too?


================
Comment at: libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp:48-51
+// So that we conform to sized_sentinel_for.
+constexpr std::ptrdiff_t operator-(const ForwardIter& x, const ForwardIter& y) {
+    return x.base() - y.base();
+}
----------------
Nit: consider making this a hidden friend.


================
Comment at: libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp:102
+
+void test(std::ranges::sentinel_t<ForwardRange>) { }
+
----------------
Please either rename or document why this function exists.


================
Comment at: libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp:111
+
+  ForwardRange fv;
+  assert(!fv.empty());
----------------
Nit: `fv` or `fr`? (I'd personally go with single-letter names to avoid the mental "what does this letter correspond to?".)


================
Comment at: libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp:155
+
+// TODO: why is the child member picked?
+//   assert(ranges::data(din) == nullptr);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101737