[libcxx-commits] [PATCH] D116894: [libc++] [ranges] Implement P2415R2 owning_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.

A couple of comments, but overall looks pretty good. Thanks!



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.all/all_t.compile.pass.cpp:19
 #include "test_iterators.h"
 #include "test_range.h"
 
----------------
You're not using this header anymore, I think.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.all/range.owning.view/base.pass.cpp:25-28
+struct Base {
+    int *begin() const;
+    int *end() const;
+};
----------------
Please make sure your indentation is consistent (either 2 or 4 spaces, but don't mix the two in the same file).


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.all/range.owning.view/begin_end.pass.cpp:28
+
+struct Base {
+  constexpr int *begin() { return nullptr; }
----------------
I found it weird to reuse the same type for testing const and non-const, and using the fact that the const versions return `char*` and the non-const versions `int*`. I was going to ask to use two separate view types instead, but reconsidering it, I think it's easier to follow as-is for some reason that escapes me. No action here, just thinking out loud.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.all/range.owning.view/begin_end.pass.cpp:90
+  }
+  {
+    using OwningView = std::ranges::owning_view<DecayChecker>;
----------------
Can you one-line comment these test cases?


================
Comment at: libcxx/test/std/ranges/range.req/range.refinements/viewable_range.compile.pass.cpp:26
 //  constructible_from<remove_cvref_t<T>, T>
-//  borrowed_range<T>
+//  lvalue_reference_t<T> || movable<remove_reference_t<T>>
 //
----------------
You are missing `initializer_list`, and also the tests for it below AFAICT.


================
Comment at: libcxx/test/std/ranges/range.req/range.refinements/viewable_range.compile.pass.cpp:120
 static_assert(!std::ranges::viewable_range<void>);
 static_assert(!std::ranges::viewable_range<int>);
 static_assert(!std::ranges::viewable_range<int (*)(char)>);
----------------
In the paper, they mention this:

>  Editor's note: `remove_reference_t` rather than `remove_cvref_t` because we need to reject const `vector<int>&&` from being a `viewable_range`

I don't think we have a test that a `Range const&&` isn't a `viewable_range` -- could we add one? In other words, I'd like a test that fails if you had used `movable<remove_cvref_t<_Tp>>` instead of `movable<remove_reference_t<_Tp>>`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116894