This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][test] Split off debug mode tests
ClosedPublic

Authored by krisb on Apr 15 2021, 12:17 PM.

Details

Reviewers
Quuxplusone
ldionne
Group Reviewers
Restricted Project
Commits
rG90248f2daa05: [libcxx][test] Split off debug mode tests
Summary

This continues the work started by @ldionne in 2908eb20ba7.

The debug mode tests from

  • libcxx/containers/sequences/vector/
  • libcxx/strings/basic.string/string.access/
  • libcxx/strings/basic.string/string.iterators/

similarly contain two tests in every file making the second test never
run. The patch splits the tests into separate files.

Diff Detail

Event Timeline

krisb created this revision.Apr 15 2021, 12:17 PM
krisb requested review of this revision.Apr 15 2021, 12:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2021, 12:17 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Apr 15 2021, 1:44 PM

Thanks a lot for doing this! Assuming you didn't make any changes to the code itself, this LGTM.

Can you please rebase onto main and re-upload. That'll trigger the CI again and you can ship this once it is green. Thanks!

This revision is now accepted and ready to land.Apr 15 2021, 1:44 PM
Quuxplusone accepted this revision.Apr 15 2021, 2:45 PM
Quuxplusone added inline comments.
libcxx/test/libcxx/containers/sequences/vector/db_back.pass.cpp
32–33

IIUC, this can/should be simply

(void)c.back();  // expect this to exit
assert(false);

But as long as you fix it in D100595, it's no problem to leave it wonky in this commit.

libcxx/test/libcxx/strings/basic.string/string.iterators/db_iterators_9.pass.cpp
14

Why is this test unsupported in C++03? Actually, what on earth is the point of this test?
I'm fine shipping this commit as-is, as long as we get to the bottom of it in D100595.
But I'd be happier with just git rm'ing this new file, because (unless I'm missing something) it's identical to db_iterators_2.pass.cpp except for the unneeded UNSUPPORTED line.

krisb added inline comments.Apr 19 2021, 5:24 AM
libcxx/test/libcxx/containers/sequences/vector/db_back.pass.cpp
32–33

Yes, I'd prefer to leave the changes like this for D100595.

libcxx/test/libcxx/strings/basic.string/string.iterators/db_iterators_9.pass.cpp
14

Thanks for pointing to this.
I didn't think much about the tests themselves while creating this patch, just extracted the second part of each test to a separate file and automatically marked them UNSUPPORTED: c++03 because they were under #if TEST_STD_VER >= 11.
But you are right, we do not need UNSUPPORTED: c++03 here (and for all the similar tests), the tests work fine with c++03 as well as with other supported versions of the standard. I guess this code was under #if TEST_STD_VER >= 11 following min_allocator requirements, which needed c++11 or newer before D67675, but I didn't dig deeper to answer why min_allocator required c++ >= 11. I'll update the tests.

Actually, what on earth is the point of this test?

This test unlike db_iterators_2.pass.cpp constructs a basic_string with a custom allocator.
Basically, it follows the same pattern as other tests for containers (both normal and debug modes) where we have one test with default parameters and one with a custom allocator. So, I'd keep this test in.

krisb updated this revision to Diff 338491.Apr 19 2021, 5:26 AM

Remove 'UNSUPPORTED: c++03' where it isn't necessary.

ldionne accepted this revision.Apr 19 2021, 5:49 AM

Still LGTM

This revision was automatically updated to reflect the committed changes.