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

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

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

LGTM % comments; thanks for cleaning up these TODOs!
(@ldionne via Discord says: "I'm fine with this!")



================
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) {
----------------
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?


================
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);
   }
 
   {
----------------
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.)


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/types.h:162
 struct sentinel_type {
-  T base;
+  T end;
 
----------------
I suggest renaming to `base_` instead. (Ditto in the other places you do this.)


================
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);
----------------
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.


================
Comment at: libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp:138
     explicit SentinelType() = default;
-    constexpr explicit SentinelType(int *base) : base_(base) {}
-    friend constexpr ResultType operator==(ForwardIter const& iter, SentinelType const& sent) noexcept { return {iter.base() == sent.base_}; }
-    friend constexpr ResultType operator==(SentinelType const& sent, ForwardIter const& iter) noexcept { return {iter.base() == sent.base_}; }
-    friend constexpr ResultType operator!=(ForwardIter const& iter, SentinelType const& sent) noexcept { return {iter.base() != sent.base_}; }
-    friend constexpr ResultType operator!=(SentinelType const& sent, ForwardIter const& iter) noexcept { return {iter.base() != sent.base_}; }
+    explicit constexpr SentinelType(int *base) : base_(base) {}
+    friend constexpr ResultType operator==(ForwardIter const& iter, SentinelType const& sent) noexcept { return {base(iter) == sent.base_}; }
----------------
Style nit: https://quuxplusone.github.io/blog/2021/04/03/static-constexpr-whittling-knife/#explicitness-all-of-your-constru


================
Comment at: libcxx/test/std/re/re.results/re.results.form/form1.pass.cpp:33-35
+        char* r = base(m.format(cpp17_output_iterator<char*>(out),
+                    fmt, fmt + std::char_traits<char>::length(fmt)));
         assert(r == out + 58);
----------------
Consider refactoring to
```
auto r = m.format(cpp17_output_iterator<char*>(out),
                    fmt, fmt + std::char_traits<char>::length(fmt));
assert(base(r) == out + 58);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120742