[libcxx-commits] [PATCH] D100255: [libcxx][ranges] adds `range` access CPOs

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/test/std/ranges/range.access/begin.h:39
+template<enable_constness Const>
+inline constexpr bool std::ranges::enable_borrowed_range<member_only<Const, true> > = true;
+
----------------
nit: maybe use `borrowed` here to match everything else, but it's nonblocking. 


================
Comment at: libcxx/test/std/ranges/range.access/begin.h:71
+
+  constexpr int const* begin() const&&
+  requires (Const != enable_constness::mutable_only) {
----------------
I guess it doesn't //hurt// to have this but at what point are you going to have a const rvalue?


================
Comment at: libcxx/test/std/ranges/range.access/begin.h:220
+template<enable_constness Const, bool is_borrowed>
+struct lvalue_adl_with_private_members : private member_only<Const, is_borrowed> {
+  constexpr friend int* begin(lvalue_adl_with_private_members& x)
----------------
Do we really need a test for this? Could you even write an implementation that picked the wrong `begin`?


================
Comment at: libcxx/test/std/ranges/range.access/begin.h:346
+template<enable_constness Const, bool>
+struct lvalue_adl_with_bad_members {
+  constexpr int begin() {
----------------
I should add a test like this for `ranges::size` :)


================
Comment at: libcxx/test/std/ranges/range.access/begin.h:346
+template<enable_constness Const, bool>
+struct lvalue_adl_with_bad_members {
+  constexpr int begin() {
----------------
zoecarver wrote:
> I should add a test like this for `ranges::size` :)
How about the reverse of this `members_with_bad_lvalue_adl`?


================
Comment at: libcxx/test/std/ranges/range.access/begin.h:430
+template<enable_constness Const, bool>
+struct rvalue_adl_with_data_member_begin {
+  constexpr friend int* begin(rvalue_adl_with_data_member_begin&& x)
----------------
For future reference: I'm not sure we need rvalue and lvalue types when testing something like this. That logic has already been tested, so one test with a data member should be sufficient. 


================
Comment at: libcxx/test/std/ranges/range.access/range.access.begin/begin.pass.cpp:165
+static_assert(!check_invocable_rvalue_adl<rvalue_adl_with_private_lvalue_members, borrowed>());
+static_assert(!check_invocable_rvalue_adl<rvalue_adl_with_private_members, borrowed>());
+
----------------
Copy paste error?


================
Comment at: libcxx/test/std/ranges/range.access/range.access.begin/begin.pass.cpp:228
+  static_assert(check_member_impl<T<enable_constness::mutable_only, is_borrowed>&>());
+  assert((check_member_impl<T<enable_constness::mutable_only, is_borrowed>&>()));
+  // static_assert(check_lvalue_member_impl<T<enable_constness::mutable_only, is_borrowed> const&>());
----------------
Why the extra parens?


================
Comment at: libcxx/test/std/ranges/range.access/range.access.begin/begin.pass.cpp:229
+  assert((check_member_impl<T<enable_constness::mutable_only, is_borrowed>&>()));
+  // static_assert(check_lvalue_member_impl<T<enable_constness::mutable_only, is_borrowed> const&>());
+  // assert((check_lvalue_member_impl<T<enable_constness::mutable_only, is_borrowed> const&>()));
----------------
Why is this commented out?


================
Comment at: libcxx/test/std/ranges/range.access/range.access.begin/incomplete.compile.verify.cpp:28
+  struct incomplete;
+  f<incomplete(&)[10]>();
+  // expected-error@*:* {{"`std::ranges::begin` is SFINAE-unfriendly on arrays of an incomplete type."}}
----------------
I think it would make more sense to call `std::ranges::begin` directly here. Certainly don't use `__begin::__fn`, especially if it's not in a libc++ test (which this probably shouldn't be anyway).


================
Comment at: libcxx/test/std/ranges/range.access/range.access.cbegin/cbegin.pass.cpp:48
+  requires(T&& t) {
+    { std::ranges::cbegin(std::forward<T>(t)) } -> std::same_as<std::decay_t<decltype(begin(std::as_const(t)))> >;
+  };
----------------
I'm generally fairly wary of tests like this where the expected value is computed. I think the best tests are the ones where the "desired result" or expected value are concrete/literal. It's easier to read and verify that way. Additionally, if there was a bug in the implementation (for example if we couldn't use `as_const` for some reason) it wouldn't be caught. 

That begin said, I don't need you to change anything here, I just wanted to point it out so you could think about this going forward. The most important check here is the `std::invocable<cbegin_t&, T>` one, and that one isn't computed (at least not in the same way). 


================
Comment at: libcxx/test/std/ranges/range.access/range.access.cbegin/cbegin.pass.cpp:112
+static_assert(check_invocable_lvalue_member<member_preferred, !borrowed>());
+static_assert(check_invocable_rvalue_member<member_preferred, !borrowed>());
+static_assert(check_invocable_lvalue_member<member_preferred, borrowed>());
----------------
I would expect this to be false. It seems like all over this test rvalues with borrowing disabled are allowed when they shouldn't be. 


================
Comment at: libcxx/test/std/ranges/range.access/range.access.cbegin/cbegin.pass.cpp:165
+static_assert(!check_invocable_rvalue_adl<rvalue_adl_with_private_lvalue_members, borrowed>());
+static_assert(!check_invocable_rvalue_adl<rvalue_adl_with_private_members, borrowed>());
+
----------------
Same as in the other file. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100255