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

[Kristina Bessonova via Phabricator via libcxx-commits]

krisb 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:
> 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?
> 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100029