This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Removes base member from tests.
ClosedPublic

Authored by Mordante on Mar 1 2022, 9:42 AM.

Details

Reviewers
ldionne
Quuxplusone
Group Reviewers
Restricted Project
Commits
rG5f26d8636f50: [libc++] Removes base member from tests.
Summary

Change the tests to use the base friend function instead of members.
Also changed some types to have a base friends instead of members.

Diff Detail

Event Timeline

Mordante created this revision.Mar 1 2022, 9:42 AM
Mordante requested review of this revision.Mar 1 2022, 9:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 9:43 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision as: ldionne.Mar 1 2022, 10:14 AM
ldionne added a subscriber: ldionne.

The direction LGTM, I think Arthur has some comments.

Quuxplusone accepted this revision.Mar 1 2022, 10:15 AM
Quuxplusone added a subscriber: Quuxplusone.

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–429

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
138
libcxx/test/std/re/re.results/re.results.form/form1.pass.cpp
33–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);
This revision is now accepted and ready to land.Mar 1 2022, 10:15 AM
Mordante marked 4 inline comments as done.Mar 1 2022, 11:16 AM

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–429

I wouldn't mind to do that in a followup.

Mordante updated this revision to Diff 412174.Mar 1 2022, 11:18 AM
Mordante marked 2 inline comments as done.

Addresses review comments.
Rebased on main to fix CI.

Mordante updated this revision to Diff 412428.Mar 2 2022, 8:19 AM

Rebased and fixes merge conflicts.
Fix CI issues by testing with all locales on my system.

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 8:19 AM
Mordante updated this revision to Diff 412608.Mar 2 2022, 10:48 PM

Rebased and attempt to fix Apple CI.

ldionne accepted this revision.Mar 3 2022, 6:20 AM

LGTM. The Apple CI failure seemed unrelated (I think it's a flake that I'll have to investigate).

Mordante updated this revision to Diff 412727.Mar 3 2022, 8:04 AM

Rebase and test whether CI passes.

This revision was automatically updated to reflect the committed changes.