[libcxx][test] Properly construct containers in debug mode tests for map/set

[krisb]

Summary

The debug mode tests for unordered map/set iterators construct empty containers,
making the code after the first increment meaningless.
It's never executed since the tests exit earlier.

It doesn't seem to be intentional, so the patch makes the tests to construct
containers that include an element.

[krisb]

Make it compiled with c++03.

[curdeius]

Very good catch indeed. Thanks.

libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp

32–33

In order not to repeat this error, could you add assert(i != c.end(c.bucket(1))); before and assert(i == c.end(c.bucket(1))); after the first increment, please?
And add assert(i != c.end()); for non-local iterators.
Please factor out c.bucket(1) too.

And a nit, 1 can be considered as a (bucket) index if read hastily. Please use some magic value, 42 or whatever, but not 0 nor 1.

[• Quuxplusone]

libcxx/test/libcxx/containers/unord/unord.multiset/db_iterators_7.pass.cpp

30–33

This PR seems like a step forward (especially once @curdeius' comments are addressed), but even after taking curdeius' comments and making the other tests match this one, don't we still have the same problem that if any libc++ assert ever fails, the test succeeds? So again here, if the ++i on line 29 assert-fails (incorrectly), then the rest of the test doesn't run and the test appears to pass (incorrectly).

Scope creep!; but I wonder if these tests should be rewritten to use something like #define _LIBCPP_ASSERT(x, m) ((x) ? (void)0 : foo(m)) and then define a foo that (1) checks for the exact message m that we expect, and (2) checks a global boolean that we set immediately before the operation that we expect to fail, so that "early failures" don't (incorrectly) count as successes.

[curdeius]

libcxx/test/libcxx/containers/unord/unord.multiset/db_iterators_7.pass.cpp

30–33

+1. Good remark.

[krisb]

Address @curdeius' comments.

[krisb]

Simplify the changes a bit.

[krisb]

@curdeius thank you for looking at this!

libcxx/test/libcxx/containers/unord/unord.multiset/db_iterators_7.pass.cpp

30–33

@Quuxplusone good point, thank you! I'll take a look.

[curdeius]

libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp

16

To answer @Quuxplusone's point 2), you can do just something like the suggested edits here... (see another comment for the rest)

Comparing the assertion messages may be a bit difficult though, and brittle.

I.e. if we wanted to do something like:

#include <assert.h>
#include <stdlib.h>
#include <string.h>

void test_assertion(const char* msg) {
    assert(::strcmp(msg, "expected") == 0);
    ::exit(::test_exit_code);
}

#define _LIBCPP_ASSERT(x, m) ((x) ? (void)0 : ::test_assertion(m))

we hit the problem that we include some headers for assert, exit and strcmp, but we redefine _LIBCPP_ASSERT afterwards.
Also these headers might get included transitively by later includes (but won't be because of header guards), but they'll have _LIBCPP_ASSERT not redefined.

An orthogonal problem is to have the actual message when assert(strcmp(...)...); fails.

34

... and then here.

[krisb]

libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp

16

we hit the problem that we include some headers for assert, exit and strcmp, but we redefine _LIBCPP_ASSERT afterwards

It doesn't seem like we need to care about assert.h, stdlib.h and other 'C'-headers here, since _LIBCPP_ASSERT isn't expected to appear in them and any includes.

So for the particular test it may look like:

#include <string.h>
#include <cassert>
int test_exit_code = 1;
#define _LIBCPP_ASSERT(x, m)                                                   \
  ((x) ? (void)0                                                               \
       : ((::strcmp(m, "Attempted to increment non-incrementable unordered "   \
                       "container local_iterator") == 0)                       \
              ? std::exit(::test_exit_code)                                    \
              : assert(x)))

and later in the test

int main(int, char**) {
    ...
    test_exit_code = 0;
    ++i; /* _LIBCPP_ASSERT is expected to fail here */
    ...

I'll test the approach and create a separate review for it.

[• Quuxplusone]

libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp

16

@krisb: Yes, that's basically what I'm asking for, except please put it in a helper function so you don't have to string all the side effects into one giant expression. E.g. this: https://godbolt.org/z/nYn1ohEen

[• Quuxplusone]

libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp

16

libcxx/test/libcxx/strings/basic.string/string.modifiers/erase_iter_db1.pass.cpp is similarly bogus. @krisb, do you have any interest in including that one in this patch as well? or making a followup patch for all such tests?

[krisb]

libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp

16

except please put it in a helper function so you don't have to string all the side effects into one giant expression.

Despite the macro looks a bit monstrous, it has one advantage that seems important to me. Doing the checks in such a way makes us possible to report the original failure message from unexpected asserts. This seems valuable. So, honestly, I'd prefer one giant expression with clear error messages than an easy-to-read helper function that is hard to understand why it failed.

do you have any interest in including that one in this patch as well? or making a followup patch for all such tests?

Yes, I'm going to update _LIBCPP_ASSERT in all the debug mode tests and fix the issues it reveals.

[• Quuxplusone]

LGTM. As I now understand the current situation: D100029 now does not change anything about _LIBCPP_ASSERT nor test/support/.
It's just changing these tests' main functions to make sure they operate on non-empty containers so that they don't prematurely hit an assert and exit "successfully failed."
I think we should land this so we can move on to D100595.
@curdeius?

[curdeius]

LGTM.

libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // <unordered_map>
 
 // Increment local_iterator past end.
 
 // UNSUPPORTED: libcxx-no-debug-mode
 
 // ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DEBUG=1
 #define _LIBCPP_ASSERT(x, m) ((x) ? (void)0 : std::exit(0))
 
 #include <unordered_map>
 #include <string>
 #include <cassert>
 
 #include "test_macros.h"
 
 int main(int, char**) {
     typedef std::unordered_map<int, std::string> C;
-    C c(1);
-    C::local_iterator i = c.begin(0);
+    typedef std::pair<int, std::string> P;
+    P a[] = {P(42, std::string())};
+    C c(std::begin(a), std::end(a));
+    C::size_type b = c.bucket(42);
+    C::local_iterator i = c.begin(b);
+    assert(i != c.end(b));
     ++i;
+    assert(i == c.end(b));
     ++i;
     assert(false);
 
     return 0;
 }

libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_9.pass.cpp

 #include <string>
 
 #include "test_macros.h"
 #include "min_allocator.h"
 
 int main(int, char**) {
     typedef std::unordered_map<int, std::string, std::hash<int>, std::equal_to<int>,
                         min_allocator<std::pair<const int, std::string>>> C;
-    C c(1);
-    C::local_iterator i = c.begin(0);
+    typedef std::pair<int, std::string> P;
+    P a[] = {P(42, std::string())};
+    C c(std::begin(a), std::end(a));
+    C::size_type b = c.bucket(42);
+    C::local_iterator i = c.begin(b);
+    assert(i != c.end(b));
     ++i;
+    assert(i == c.end(b));
     ++i;
     assert(false);
 
     return 0;
 }

libcxx/test/libcxx/containers/unord/unord.multimap/db_local_iterators_7.pass.cpp

 #include <unordered_map>
 #include <cassert>
 #include <string>
 
 #include "test_macros.h"
 
 int main(int, char**) {
     typedef std::unordered_multimap<int, std::string> C;
-    C c(1);
-    C::local_iterator i = c.begin(0);
+    typedef std::pair<int, std::string> P;
+    P a[] = {P(42, std::string())};
+    C c(std::begin(a), std::end(a));
+    C::size_type b = c.bucket(42);
+    C::local_iterator i = c.begin(b);
+    assert(i != c.end(b));
     ++i;
+    assert(i == c.end(b));
     ++i;
     assert(false);
 
     return 0;
 }

libcxx/test/libcxx/containers/unord/unord.multimap/db_local_iterators_9.pass.cpp

 #include <utility>
 
 #include "test_macros.h"
 #include "min_allocator.h"
 
 int main(int, char**) {
     typedef std::unordered_multimap<int, std::string, std::hash<int>, std::equal_to<int>,
                         min_allocator<std::pair<const int, std::string>>> C;
-    C c(1);
-    C::local_iterator i = c.begin(0);
+    typedef std::pair<int, std::string> P;
+    P a[] = {P(42, std::string())};
+    C c(std::begin(a), std::end(a));
+    C::size_type b = c.bucket(42);
+    C::local_iterator i = c.begin(b);
+    assert(i != c.end(b));
     ++i;
+    assert(i == c.end(b));
     ++i;
     assert(false);
 
     return 0;
 }

libcxx/test/libcxx/containers/unord/unord.multiset/db_iterators_7.pass.cpp

 #include <unordered_set>
 #include <cassert>
 
 #include "test_macros.h"
 
 int main(int, char**) {
     typedef int T;
     typedef std::unordered_multiset<T> C;
-    C c(1);
+    T a[] = {42};
+    C c(std::begin(a), std::end(a));
     C::iterator i = c.begin();
+    assert(i != c.end());
     ++i;
     assert(i == c.end());
     ++i;
     assert(false);
 
     return 0;
 }

libcxx/test/libcxx/containers/unord/unord.multiset/db_iterators_9.pass.cpp

 #include <functional>
 
 #include "test_macros.h"
 #include "min_allocator.h"
 
 int main(int, char**) {
     typedef int T;
     typedef std::unordered_multiset<T, std::hash<T>, std::equal_to<T>, min_allocator<T>> C;
-    C c(1);
+    T a[] = {42};
+    C c(std::begin(a), std::end(a));
     C::iterator i = c.begin();
+    assert(i != c.end());
     ++i;
     assert(i == c.end());
     ++i;
     assert(false);
 
     return 0;
 }

libcxx/test/libcxx/containers/unord/unord.multiset/db_local_iterators_7.pass.cpp

 #include <unordered_set>
 #include <cassert>
 
 #include "test_macros.h"
 
 int main(int, char**) {
     typedef int T;
     typedef std::unordered_multiset<T> C;
-    C c(1);
-    C::local_iterator i = c.begin(0);
+    T a[] = {42};
+    C c(std::begin(a), std::end(a));
+    C::size_type b = c.bucket(42);
+    C::local_iterator i = c.begin(b);
+    assert(i != c.end(b));
     ++i;
+    assert(i == c.end(b));
     ++i;
     assert(false);
 
     return 0;
 }

libcxx/test/libcxx/containers/unord/unord.multiset/db_local_iterators_9.pass.cpp

 #include <functional>
 
 #include "test_macros.h"
 #include "min_allocator.h"
 
 int main(int, char**) {
     typedef int T;
     typedef std::unordered_multiset<T, std::hash<T>, std::equal_to<T>, min_allocator<T>> C;
-    C c(1);
-    C::local_iterator i = c.begin(0);
+    T a[] = {42};
+    C c(std::begin(a), std::end(a));
+    C::size_type b = c.bucket(42);
+    C::local_iterator i = c.begin(b);
+    assert(i != c.end(b));
     ++i;
+    assert(i == c.end(b));
     ++i;
     assert(false);
 
     return 0;
 }

libcxx/test/libcxx/containers/unord/unord.set/db_iterators_7.pass.cpp

 #include <unordered_set>
 #include <cassert>
 
 #include "test_macros.h"
 
 int main(int, char**) {
     typedef int T;
     typedef std::unordered_set<T> C;
-    C c(1);
+    T a[] = {42};
+    C c(std::begin(a), std::end(a));
     C::iterator i = c.begin();
+    assert(i != c.end());
     ++i;
     assert(i == c.end());
     ++i;
     assert(false);
 
     return 0;
 }

libcxx/test/libcxx/containers/unord/unord.set/db_iterators_9.pass.cpp

 #include <functional>
 
 #include "test_macros.h"
 #include "min_allocator.h"
 
 int main(int, char**) {
     typedef int T;
     typedef std::unordered_set<T, std::hash<T>, std::equal_to<T>, min_allocator<T>> C;
-    C c(1);
+    T a[] = {42};
+    C c(std::begin(a), std::end(a));
     C::iterator i = c.begin();
+    assert(i != c.end());
     ++i;
     assert(i == c.end());
     ++i;
     assert(false);
 
     return 0;
 }

libcxx/test/libcxx/containers/unord/unord.set/db_local_iterators_7.pass.cpp

 #include <unordered_set>
 #include <cassert>
 
 #include "test_macros.h"
 
 int main(int, char**) {
     typedef int T;
     typedef std::unordered_set<T> C;
-    C c(1);
-    C::local_iterator i = c.begin(0);
+    T a[] = {42};
+    C c(std::begin(a), std::end(a));
+    C::size_type b = c.bucket(42);
+    C::local_iterator i = c.begin(b);
+    assert(i != c.end(b));
     ++i;
+    assert(i == c.end(b));
     ++i;
     assert(false);
 
     return 0;
 }

libcxx/test/libcxx/containers/unord/unord.set/db_local_iterators_9.pass.cpp

 #include <functional>
 
 #include "test_macros.h"
 #include "min_allocator.h"
 
 int main(int, char**) {
     typedef int T;
     typedef std::unordered_set<T, std::hash<T>, std::equal_to<T>, min_allocator<T>> C;
-    C c(1);
-    C::local_iterator i = c.begin(0);
+    T a[] = {42};
+    C c(std::begin(a), std::end(a));
+    C::size_type b = c.bucket(42);
+    C::local_iterator i = c.begin(b);
+    assert(i != c.end(b));
     ++i;
+    assert(i == c.end(b));
     ++i;
     assert(false);
 
     return 0;
 }