[libcxx-commits] [PATCH] D105753: [libcxx][ranges] Add `ranges::common_view`.

[Louis Dionne via Phabricator via libcxx-commits]

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

Per OTS review.



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/base.pass.cpp:59
+  {
+    std::ranges::common_view<CopyableView> common(CopyableView{buffer});
+    assert(common.base().ptr_ == buffer);
----------------
Could you store the result of `.base()` in a variable just so we get an explicit assertion of what the return type of the function is?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp:72-77
+constexpr auto operator-(sentinel_wrapper<ForwardIter> sent, ForwardIter iter) {
+  return sent.base().base() - iter.base();
+}
+constexpr auto operator-(ForwardIter iter, sentinel_wrapper<ForwardIter> sent) {
+  return iter.base() - sent.base().base();
+}
----------------
Are those only for convenience? If so, they should be removed, otherwise, can you please add a one-liner comment about why they are needed? (same below)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp:113
+
+  {
+    std::ranges::common_view<SizedForwardView> comm(SizedForwardView{buffer});
----------------
We should not be testing `begin()` by dereferencing the resulting iterator. Instead, we should compare the iterator itself. Is that possible or am I missing something?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/begin.pass.cpp:115
+    std::ranges::common_view<SizedForwardView> comm(SizedForwardView{buffer});
+    if (!std::is_constant_evaluated())
+      assert(*comm.begin() == 1);
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > Quuxplusone wrote:
> > > Why do we need this `if`? What piece is not constexpr-friendly here?
> > Basically none of common_iterator is constexpr. //Sigh//.
> I see. Ugh. The constructor and accessors of `common_view` are constexpr, but `common_iterator` isn't. OK.
Instead of doing this `if (!is_constant_evaluated())`, I would make the test non-constexpr and add a simpler constexpr test that only calls `begin()` (const and non-const) in a constexpr context.

Then, when we go back and make `common_iterator` constexpr-friendly, we'll only have to remove that tiny test and make the whole test constexpr.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/borrowing.compile.pass.cpp:19
+
+#include "test_macros.h"
+#include "test_iterators.h"
----------------
Not needed. (might apply elsewhere too)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/ctad.compile.pass.cpp:58
+    >);
+    // std::ranges::common_view(std::move(r)) is ill-formed
+    static_assert(std::same_as<
----------------
Is it ill-formed, or should it SFINAE away? I think it's the latter, right? If so, we can use a `requires` to check it.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.common.view/ctor.pass.cpp:13-14
+
+// common_view() requires default_initializable<V> = default;
+// constexpr explicit common_view(V r);
+
----------------
I would like to split this into `ctor.default.pass.cpp` and `ctor.view.pass.cpp` for consistency w/ the rest of the test suite.


================
Comment at: libcxx/test/support/test_iterators.h:927
 
+  constexpr I base() const { return base_; }
+
----------------
`constexpr I const& base() const`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105753