[libcxx-commits] [PATCH] D119214: [libc++][ranges][NFC] Refactor tests for `ranges::{begin, end}`.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

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

LGTM % comments (thanks for the cleanup!) but the merge-conflict comment is significant.



================
Comment at: libcxx/test/std/ranges/range.access/begin.pass.cpp:32-41
+static_assert(!std::is_invocable_v<RangeCBeginT, int (&&)[10]>);
+static_assert( std::is_invocable_v<RangeCBeginT, int (&)[10]>);
+static_assert(!std::is_invocable_v<RangeCBeginT, int (&&)[]>);
+static_assert( std::is_invocable_v<RangeCBeginT, int (&)[]>);
 
 struct Incomplete;
 static_assert(!std::is_invocable_v<RangeBeginT, Incomplete(&&)[]>);
----------------
Looks like you had a merge conflict here. Please take what's on the left-hand side.
Your new lines 39 and 41 are the IFNDR case (right?), so we can't `static_assert` anything about them; we must `LIBCPP_STATIC_ASSERT` as seen on the left-hand side.


================
Comment at: libcxx/test/std/ranges/range.access/begin.pass.cpp:194-197
+struct BeginFunctionReturnsEmpty {
   struct Empty {};
-  Empty x;
-  friend constexpr const Empty *begin(BeginFunctionReturnsEmptyPtr const& bf) { return &bf.x; }
+  friend Empty begin(BeginFunctionReturnsEmpty const&);
 };
----------------
FWIW, there's no difference between `BeginFunctionReturnsInt` and `BeginFunctionReturnsEmpty`, so I'd be perfectly happy to remove one or the other (and all else being equal, I'd remove `BeginFunctionReturnsEmpty` because then I could perhaps also remove `Empty`).
(I //do// think it's worth testing the pointer-but-not-iterator type `void*` separately from a not-even-pointer non-iterator type like `int` or `Empty`.)


================
Comment at: libcxx/test/std/ranges/range.access/end.pass.cpp:155
+};
+static_assert(!std::is_invocable_v<RangeEndT, EmptyEndMember const&>);
+
----------------
FWIW, here the main problem is that `iterator_t<EmptyEndMember>` is ill-formed because `Empty` is not an iterator type; we never even really get around to checking whether `t.end()` is well-formed. So I question the usefulness of this test case. But it's harmless.


================
Comment at: libcxx/test/std/ranges/range.access/end.pass.cpp:257
+
+struct EndFunctionWithPrivateEndMember : private EndMember {
+  int y;
----------------
Twice now I've looked at this PR, and twice I've been fooled by this "clever" line — "What? There's no private member here, just two friends!" Consider eliminating the inheritance relationship here, so that this code will more closely resemble `BeginFunctionWithPrivateBeginMember` above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119214