[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))
 
----------------
Quuxplusone wrote:
> 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
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100029