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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


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


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

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.


================
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();
+}
----------------
cjdb wrote:
> Nit: consider making this a hidden friend.
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). 


================
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);
----------------
cjdb wrote:
> 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?
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101737