[libcxx-commits] [PATCH] D140651: [libc++][format] Adds new test macros.

[Louis Dionne via Phabricator via libcxx-commits]

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

I like where this is going -- we have a few places in the test suite where we want to have nice error messages but we don't have the machinery to. Centralizing this will also make it easier to have good error messages when the platform supports it, and no error message when it doesn't (e.g. when localization or IO are not supported).



================
Comment at: libcxx/test/support/test_validation.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
I would call this `assert_macros.h`, I think. This would make a parallel with `test_macros.h`.

In fact it would be nice if those test macros could move to `test_macros.h`, but they can't because they have too many dependencies. I think this is something we should try to address separately.


================
Comment at: libcxx/test/support/test_validation.h:34
+#  include <iostream>
+#endif // TEST_HAS_NO_LOCALIZATION
+
----------------



================
Comment at: libcxx/test/support/test_validation.h:41
+concept test_char_streamable = requires(T&& value) { std::cerr << std::forward<T>(value); };
+#  endif // TEST_HAS_NO_LOCALIZATION
+
----------------



================
Comment at: libcxx/test/support/test_validation.h:51
+template <class... Args>
+[[noreturn]] void test_log_error(const char* condition, const char* file, int line, [[maybe_unused]] Args&&... args) {
+  const char* msg = condition ? "Assertion failure: " : "Unconditional failure:";
----------------
If we dropped the free-form arguments from here and instead expected a string (or a C string), we could drop the dependency on `iostream` and make this much more widely useful in the test suite. It may also become useful for older tests. You'd probably want to define another helper function that will stringify your arguments and you could use that in your tests.


================
Comment at: libcxx/test/support/test_validation.h:53
+  const char* msg = condition ? "Assertion failure: " : "Unconditional failure:";
+  fprintf(stderr, "%s%s %s %d\n", msg, condition, file, line);
+#  ifndef TEST_HAS_NO_LOCALIZATION
----------------
Here and elsewhere.


================
Comment at: libcxx/test/support/test_validation.h:63
+
+inline constexpr auto test_fail = []<class... Args>(const char* file, int line, Args&&... args) {
+  test_log_error("", file, line, std::forward<Args>(args)...);
----------------
I would make those normal function templates instead. Otherwise one wonders whether there's a  reason to define them this way.


================
Comment at: libcxx/test/support/test_validation.h:84
+      test_require(condition, condition_str, file, line, std::forward<Args>(args)...);
+#  endif // _LIBCPP_VERSION
+    };
----------------



================
Comment at: libcxx/test/support/test_validation.h:88
+// assert(false) replacement
+#  define FAIL(...) test_fail(__FILE__, __LINE__ __VA_OPT__(, ) __VA_ARGS__)
+
----------------
All of these macros should fully qualify what they call.


================
Comment at: libcxx/test/support/test_validation.h:91
+// assert replacement.
+#  define REQUIRE(CONDITION, ...) test_require(CONDITION, #CONDITION, __FILE__, __LINE__ __VA_OPT__(, ) __VA_ARGS__)
+
----------------
I think I would name the macros this way:

```
TEST_FAIL(...)
TEST_ASSERT(...)
TEST_LIBCPP_ASSERT(...)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140651