[libcxx-commits] [PATCH] D100029: [libcxx][test] Properly construct containers in debug mode tests for map/set

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/libcxx/containers/unord/unord.map/db_local_iterators_7.pass.cpp:16
 // ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DEBUG=1
 #define _LIBCPP_ASSERT(x, m) ((x) ? (void)0 : std::exit(0))
 
----------------
krisb wrote:
> curdeius wrote:
> > 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.
> >  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.
@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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100029/new/

https://reviews.llvm.org/D100029