[libcxx-commits] [PATCH] D100595: [libcxx][test] Attempt to make debug mode tests more bulletproof
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Apr 15 13:27:47 PDT 2021
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
More information about the libcxx-commits
mailing list