[libcxx-commits] [PATCH] D100595: [libcxx][test] Attempt to make debug mode tests more bulletproof

[Kristina Bessonova via Phabricator via libcxx-commits]

krisb added inline comments.


================
Comment at: libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp:19
+#include <cassert>
+#define _LIBCPP_ASSERT(x, m) assert(x)
 
----------------
curdeius wrote:
> ldionne wrote:
> > Why is this one not like the others? There's a few other files that do that too.
> Isn't it because all those move tests actually check a correct, defined behaviour. I.e. moving a container doesn't invalidate the iterators that pointed to its elements (and IIUC only end iterator is invalidated in this situation but that seems logical)?
As @curdeius correctly mention below these tests are not expected to fail, so basically we do not need to redefine _LIBCPP_ASSERT for them at all. I'll clean this up (forgot about this).


================
Comment at: libcxx/test/support/debug_macros.h:12
+
+bool expected_fail = false;
+
----------------
Quuxplusone wrote:
> ldionne wrote:
> > ```
> > namespace {
> >   bool expected_fail = false;
> > }
> > ```
> > 
> > Otherwise, we have ODR violations in tests where we compile multiple TUs.
> Even better: `static bool`.
> And it'd be good to name it //something// that connotes testiness and the fact that it's related to `TEST_LIBCPP_ASSERT`.
> Finally, I suggest that this should actually be a `const char *` that we set to the expected message; that way, we don't need to redefine `_LIBCPP_ASSERT` differently in each different test file.
> 
> ```
> // copyright header
> // header guard for "replace_libcpp_assert.h"
> 
> // This file must be included before any other files, even "test_macros.h",
> // since it affects the contents of <__debug>.
> 
> static const char *test_expect_libcpp_assert = 0;
> #define _LIBCPP_ASSERT(x, m) do { \
>     if (!test_expect_libcpp_assert) { \
>       _LIBCPP_ASSERT_IMPL(x, m); \
>     } else if (x) { \
>     } else if (std::strcmp(m, test_expect_libcpp_assert) == 0) { \
>       std::exit(0); \
>     } else { \
>       _LIBCPP_ASSERT_IMPL(false, m); \
>     } \
>   } while (0)
> ```
> Used like this:
> ```
> // copyright header
> #include "replace_libcpp_assert.h"
> #include "test_macros.h"
> ~~~
> 
> int main(~~~) {
>   C::iterator i = c.begin();
>   ++i;
>   assert(i == c.end());
>   test_expect_libcpp_assert = "Attempted to increment non-incrementable iterator";
>   ++i;
>   assert(false);
> }
> ```
If to keep these tests in at least as a short-term solution, it may make sense to reuse the logic we already have in __debug/debug.cpp. I didn't notice it early, but it seems to be the best way to handle things. I'll attempt to implement an approach in the next update. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100595