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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Generally LGTM, with some comments!



================
Comment at: libcxx/include/CMakeLists.txt:43-45
   __ranges/view.h
   __ranges/size.h
+  __ranges/view_interface.h
----------------
Should be sorted!


================
Comment at: libcxx/include/__ranges/view_interface.h:38
+class view_interface : public view_base {
+  [[nodiscard]] constexpr _Derived& __derived() noexcept {
+    return static_cast<_Derived&>(*this);
----------------
Can drop `nodiscard`.


================
Comment at: libcxx/include/__ranges/view_interface.h:55
+
+  template<class _D2 = _Derived>
+  [[nodiscard]] constexpr bool empty() const
----------------
Why is that necessary? Is that the workaround for `gcc -fconcepts` you're discussing in the comments below?


================
Comment at: libcxx/include/__ranges/view_interface.h:149
+    _LIBCPP_ASSERT(!empty(), "");
+    return *(--ranges::end(__derived()));
+  }
----------------
cjdb wrote:
> zoecarver wrote:
> > Review note: this is used until `ranges::prev` is implemented.
> Thanks for highlighting this, was about to comment.
This can be changed now (and also in the `noexcept(...)` above).


================
Comment at: libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp:12
+// UNSUPPORTED: gcc-10
+// XFAIL: msvc && clang
+
----------------
I don't think this is necessary anymore.


================
Comment at: libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp:184
+
+// TODO: why is the child member picked?
+//   assert(ranges::data(dataNull) == nullptr);
----------------
Can you elaborate on what this TODO means?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101737