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

Zoe Carver via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon May 3 09:45:01 PDT 2021

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
+  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. 

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list