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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks a lot for finding this out and trying to solve it! I think this is way superior to what we did previously, which was very naive indeed. I do have some comments, but I don't see a reason why we wouldn't move forward with this once they are resolved.



================
Comment at: libcxx/test/libcxx/containers/sequences/list/list.cons/db_move.pass.cpp:19
+#include <cassert>
+#define _LIBCPP_ASSERT(x, m) assert(x)
 
----------------
Why is this one not like the others? There's a few other files that do that too.


================
Comment at: libcxx/test/libcxx/containers/sequences/list/list.ops/db_splice_pos_list_iter_iter.pass.cpp:17
+
+// FIXME: it doesn't seem like the right assert is caught here.
+#include "debug_macros.h"
----------------
Any chance we can fix the issue in this patch or in a patch *before* we land this one?


================
Comment at: libcxx/test/libcxx/strings/basic.string/string.iterators/db_iterators_5.pass.cpp:21
+                     "Attempted to add/subtract "                                                                      \
+                     "iterator outside of valid range")
 
----------------
Unnecessary break?


================
Comment at: libcxx/test/support/debug_macros.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
This needs a header guard please!


================
Comment at: libcxx/test/support/debug_macros.h:10
+#include <cassert>
+#include <string.h>
+
----------------
You can `#include <cstring>` and then use `std::strcmp` & friends.


================
Comment at: libcxx/test/support/debug_macros.h:12
+
+bool expected_fail = false;
+
----------------
```
namespace {
  bool expected_fail = false;
}
```

Otherwise, we have ODR violations in tests where we compile multiple TUs.


================
Comment at: libcxx/test/support/debug_macros.h:14
+
+#define TEST_LIBCPP_ASSERT(x, m, e) ((x) ? (void)0 : ((expected_fail && 0 == ::strcmp(m, e)) ? ::exit(0) : assert(x)))
+
----------------
You're evaluating `x` twice, which is wrong if there's a side effect. I suggest:

```
inline void test_libcpp_assert(bool expr, char const* actual_message, char const* expected_message) {
  if (!result) {                                                                 
    if (::expected_fail && std::strcmp(actual_message, expected_message) == 0) { 
      std::exit(0);                                                              
    } else {                                                                     
      assert(false && "test failed in an unexpected manner");                    
    }                                                                            
  }
}
```

Then, in the tests, you could use:

```
#define _LIBCPP_ASSERT(expr, message) ::test_libcpp_assert(expr, message, "Attempted to compare incomparable iterators")
```



================
Comment at: libcxx/test/support/debug_macros.h:16-19
+#define XFAIL(x)                                                                                                       \
+  expected_fail = true;                                                                                                \
+  (void)(x);                                                                                                           \
+  assert(false)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100595