[libcxx-commits] [PATCH] D120742: [libc++] Removes base member from tests.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 4 inline comments as done.
Mordante added a comment.

Thanks for the reviews!



================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/assign.pass.cpp:32-41
+    friend constexpr int *base(const AssignableFromIter& i) {return i.it_;}
 
     AssignableFromIter() = default;
     explicit constexpr AssignableFromIter(int *it) : it_(it) {}
-    constexpr AssignableFromIter(const forward_iterator<int*>& it) : it_(it.base()) {}
+    constexpr AssignableFromIter(const forward_iterator<int*>& it) : it_(base(it)) {}
 
     constexpr AssignableFromIter& operator=(const forward_iterator<int*> &other) {
----------------
Quuxplusone wrote:
> Since this is just an ad-hoc iterator used in this one test, I'd prefer to s/class/struct/, make the `it_` member public, and then use `x.it_` instead of `base(x)` throughout. The only reason we like to use `base(x)` in general is for genericity — it works for both raw pointers and test-iterators. Here, IIUC, we care //only// about this one type, no?
I prefer to keep it as is since the test also uses a `forward_iterator` (line 72). Having base for this iterator makes that part of the test using one style instead of mixing `base(foo)` and `foo.it_`.


================
Comment at: libcxx/test/std/iterators/predef.iterators/counted.iterator/ctor.iter.pass.cpp:60-82
   {
     const std::counted_iterator iter(cpp20_input_iterator<int*>{buffer}, 8);
-    assert(iter.base().base() == buffer);
+    assert(base(iter.base()) == buffer);
     assert(iter.count() == 8);
   }
 
   {
----------------
Quuxplusone wrote:
> Perhaps in a separate commit, but how about you just eliminate lines 60-82? They're completely redundant with lines 36-58. (The only difference is whether the variable being constructed is `const`; but the ctor being tested here cannot possibly care about its target's constness.)
Good point, but I prefer a separate commit.


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/types.h:162
 struct sentinel_type {
-  T base;
+  T end;
 
----------------
Quuxplusone wrote:
> I suggest renaming to `base_` instead. (Ditto in the other places you do this.)
I was doubting between `end` and `base_` so will change it.


================
Comment at: libcxx/test/std/ranges/range.access/rbegin.pass.cpp:426-429
+  assert(base(std::ranges::rbegin(a).base()) == &a.e);
+  assert(base(std::ranges::crbegin(a).base()) == &a.ce);
+  assert(base(std::ranges::rbegin(aa).base()) == &aa.ce);
+  assert(base(std::ranges::crbegin(aa).base()) == &aa.ce);
----------------
Quuxplusone wrote:
> Honestly these should all be like
> ```
> assert(std::ranges::rbegin(a) == std::reverse_iterator(bidirectional_iterator<int*>(a.e)));
> ```
> but since this transformation is purely mechanical, no action required AFAIC.
I wouldn't mind to do that in a followup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120742