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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.


================
Comment at: libcxx/test/libcxx/strings/basic.string/string.iterators/db_iterators_4.pass.cpp:32
   assert(i[0] == 0);
-  assert(i[1] == 0);
+  XFAIL(assert(i[1] == 0));
   assert(false);
----------------
You forgot to remove the `assert` here. (Grep `XFAIL(assert` for more instances.)


================
Comment at: libcxx/test/support/debug_macros.h:12
+
+bool expected_fail = false;
+
----------------
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);
}
```


================
Comment at: libcxx/test/support/debug_macros.h:16-19
+#define XFAIL(x)                                                                                                       \
+  expected_fail = true;                                                                                                \
+  (void)(x);                                                                                                           \
+  assert(false)
----------------
ldionne wrote:
> Actually, another possibility which I think I like even more, is this:
> 
> ```
> template <typename F>
> [[noreturn]] void xfail(F const& f) {
>   expected_fail = true;
>   f();
>   assert(false); 
> }
> ```
> 
> Then, you can call it like
> 
> ```
> int main(int, char**) {
>   typedef std::string C;
>   C c(1, '\0');
>   C::iterator i = c.end();
>   xfail([&i]{ (void*i; });
>   return 0;
> }
> ```
> 
> It requires C++11, however I think most of the debug tests already do anyway, so that might be acceptable.
I'm concerned that any variation on `XFAIL` is too easy to confuse with the notion of an `XFAIL`ed test case. Personally I would rather just see these three lines written out in each test case. I show above my suggestion on how to implement that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100595