Change the tests to use the base friend function instead of members.
Also changed some types to have a base friends instead of members.
Details
- Reviewers
ldionne • Quuxplusone - Group Reviewers
Restricted Project - Commits
- rG5f26d8636f50: [libc++] Removes base member from tests.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
LGTM % comments; thanks for cleaning up these TODOs!
(@ldionne via Discord says: "I'm fine with this!")
libcxx/test/std/iterators/predef.iterators/counted.iterator/assign.pass.cpp | ||
---|---|---|
32–41 | 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? | |
libcxx/test/std/iterators/predef.iterators/counted.iterator/ctor.iter.pass.cpp | ||
60–82 | 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.) | |
libcxx/test/std/iterators/predef.iterators/iterators.common/types.h | ||
162 | I suggest renaming to base_ instead. (Ditto in the other places you do this.) | |
libcxx/test/std/ranges/range.access/rbegin.pass.cpp | ||
426–430 | 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. | |
libcxx/test/std/ranges/range.utility/view.interface/view.interface.pass.cpp | ||
139–142 | ||
libcxx/test/std/re/re.results/re.results.form/form1.pass.cpp | ||
32–35 | 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); |
Thanks for the reviews!
libcxx/test/std/iterators/predef.iterators/counted.iterator/assign.pass.cpp | ||
---|---|---|
32–41 | 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_. | |
libcxx/test/std/iterators/predef.iterators/counted.iterator/ctor.iter.pass.cpp | ||
60–82 | Good point, but I prefer a separate commit. | |
libcxx/test/std/iterators/predef.iterators/iterators.common/types.h | ||
162 | I was doubting between end and base_ so will change it. | |
libcxx/test/std/ranges/range.access/rbegin.pass.cpp | ||
426–430 | I wouldn't mind to do that in a followup. |
Rebased and fixes merge conflicts.
Fix CI issues by testing with all locales on my system.
LGTM. The Apple CI failure seemed unrelated (I think it's a flake that I'll have to investigate).
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?