This is an archive of the discontinued LLVM Phabricator instance.

[libc++][tests] Fixing some issues in unordered container tests
AbandonedPublic

Authored by iid_iunknown on Sep 26 2016, 3:07 PM.

Details

Reviewers
None
Summary

Hi Eric,

Would you review these minor corrections for the issues found while running the libc++ tests with _LIBCPP_DEBUG, please?

Below is the summary explaining the motivation for the changes.

(1)

  • test/std/containers/unord/unord.multimap/db_local_iterators_7.pass.cpp
  • test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp

    The tests expect the 1st iterator increment to be successful and the 2nd one to trigger _LIBCPP_ASSERT. Actually the assert is triggered by the 1st increment as the container contains no elements. Fix: initialize the container with one element.

(2)

  • test/std/containers/unord/unord.multiset/db_iterators_8.pass.cpp
  • test/std/containers/unord/unord.multiset/db_local_iterators_8.pass.cpp
  • test/std/containers/unord/unord.set/db_iterators_8.pass.cpp
  • test/std/containers/unord/unord.set/db_local_iterators_8.pass.cpp

    Container template instantiation passes the allocator type in the wrong place.

(3)

  • test/std/containers/unord/unord.multiset/db_iterators_7.pass.cpp
  • test/std/containers/unord/unord.multiset/db_local_iterators_7.pass.cpp
  • test/std/containers/unord/unord.set/db_iterators_7.pass.cpp
  • test/std/containers/unord/unord.set/db_local_iterators_7.pass.cpp

    Same as (1) + (2)

(4)

  • test/std/containers/unord/unord.set/emplace_hint.pass.cpp
  • test/std/containers/unord/unord.set/insert_hint_const_lvalue.pass.cpp
  • test/std/containers/unord/unord.set/insert_hint_rvalue.pass.cpp

    Saves the end iterator and then modifies the container invalidating the iterator due to rehashing.

Thanks!

Diff Detail

Repository
rL LLVM

Event Timeline

iid_iunknown retitled this revision from to [libc++][tests] Fixing some issues in unordered container tests.
iid_iunknown updated this object.
iid_iunknown added a reviewer: EricWF.
iid_iunknown set the repository for this revision to rL LLVM.
iid_iunknown added a subscriber: llvm-commits.
iid_iunknown updated this object.
EricWF edited edge metadata.Sep 26 2016, 4:10 PM

Thanks for cleaning up our debug mode. I've added a couple of inline comments which need to be addressed.

If your interested there is much more work to do. For example moving all debug tests from test/std to test/libcxx, or fixing the debug tests to manually #define _LIBCPP_DEBUG 1.

test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp
0–1

This uses C++11 features but is required to work in C++03. Please make sure these changes don't break C++03 tests.

Side note: Wouldn't removing the second increment be simpler than changing the size?

0–1

All of the code below this is logically dead. Please either (A) remove it, or (B) define _LIBCPP_ASSERT so it doesn't exit when invoked. For example:

#define _LIBCPP_ASSERT(p, m) ((p) ? (void)0 : throw 0)
int main()
{
    typedef std::unordered_map<int, std::string> C;
    C c(1);
    C::local_iterator i = c.begin(0);
    try {
      ++i;
      assert(false);
    } catch(int) {}
    // More tests can be written below
}
0–1

Tests for debug mode should manually enable it, instead of requiring it already be enabled. If you want to do more cleanup please change this test to #define _LIBCPP_DEBUG 1 at the start of the file, and then remove the redundant empty main.

test/std/containers/unord/unord.multiset/db_iterators_7.pass.cpp
0–1

Nice fix. Thanks!

test/std/containers/unord/unord.set/insert_hint_const_lvalue.pass.cpp
0–1

These fixes LGTM. Thanks.

iid_iunknown edited edge metadata.
iid_iunknown removed rL LLVM as the repository for this revision.

Addressing the review remarks

iid_iunknown marked 5 inline comments as done.Sep 27 2016, 2:19 PM

Thanks for cleaning up our debug mode. I've added a couple of inline comments which need to be addressed.

Thank you for looking into this, Eric! Much appreciated.

Please find my inline answers below.

If your interested there is much more work to do. For example moving all debug tests from test/std to test/libcxx, or fixing the debug tests to manually #define _LIBCPP_DEBUG 1.

Yes, I can do this once I am done with my other tasks. May we do this refactoring in a separate review request to avoid a relatively big commit?

test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp
0–1

Done. Several other tests under test/libcxx have also been fixed.

0–1

This uses C++11 features but is required to work in C++03.

Thanks for catching this! Fixed.

Wouldn't removing the second increment be simpler than changing the size?

My thought was that these 2 increments were added intentinally to test successful and unsuccessful increment. On the other hand, other tests already check successful increments and retesting them here is superfluous. Removing element insertion and one increment according to your advice.

0–1

Agree. We build all the libc++ tests into a single executable binary to test on our system. We also have custom assert, exit() / abort() and _LIBCPP_ASSERT (similar to the one you suggested) so that test failures do not stop execution. Thus our testing env is not affected, which is why I didn't fix this. The usual test workflow does suffer from this however. Fixing these and some other tests according to your advice.

iid_iunknown marked 3 inline comments as done.Sep 27 2016, 2:21 PM
iid_iunknown added inline comments.
test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp
0–1

Agree. We build all the libc++ tests into a single executable binary to test on our system. We also have custom assert, exit() / abort() and _LIBCPP_ASSERT (similar to the one you suggested) so that test failures do not stop execution. Thus our testing env is not affected, which is why I didn't fix this. The usual test workflow does suffer from this however. Fixing these and some other tests according to your advice.

EricWF added a comment.Oct 7 2016, 3:17 PM

This *almost* LGTM. The tests that have been converted to a throwing _LIBCPP_ASSERT need to be marked with // UNSUPPORTED: libcpp-no-exceptions

Yes, I can do this once I am done with my other tasks. May we do this refactoring in a separate review request to avoid a relatively big commit?

Absolutely. Refactorings are always welcome as multiple smaller patches.

iid_iunknown set the repository for this revision to rL LLVM.

Addressing the review remarks.
"UNSUPPORTED: libcpp-no-exceptions" added.

This *almost* LGTM. The tests that have been converted to a throwing _LIBCPP_ASSERT need to be marked with // UNSUPPORTED: libcpp-no-exceptions

Yep, I was going to add UNSUPPORTED but accidentally missed them at the last moment.
Thanks for the reminder!

This almost LGTM. I would like to see the remaining _db tests moved into the test/libcxx subdirectory and I would like to see them manually define _LIBCPP_DEBUG instead of guarding for when it is not defined.

Note this should only be done for tests that are entirely debug tests and which test no non-debug behavior.

test/std/containers/sequences/vector/vector.modifiers/erase_iter_db2.pass.cpp
16

For tests like this, where the entire test is a debug test (guarded by #if _LIBCPP_DEBUG >= N) please do two things:

  1. Move the test from test/std/.../foo/... to test/libcxx/.../foo/....
  2. Manually define _LIBCPP_DEBUG instead of checking for it. This way the debug test always run.
EricWF resigned from this revision.Dec 27 2016, 10:42 PM
EricWF removed a reviewer: EricWF.

Resigning as reviewer. I actually removed and entirely re-implemented these tests earlier today. This revision should be abandoned.

I'm very sorry about steamrolling over this. Thank you very much for the patch. I hope this doesn't discourage future contributions.

iid_iunknown abandoned this revision.Dec 28 2016, 5:45 AM