[libcxx-commits] [PATCH] D142808: [libc++][test] Adds more generic test macros.
Mark de Wever via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Feb 6 05:03:47 PST 2023
Mordante created this revision.
Herald added a subscriber: jdoerfert.
Herald added a project: All.
Mordante updated this revision to Diff 492998.
Mordante added a comment.
Mordante marked 8 inline comments as done.
Mordante updated this revision to Diff 493712.
Mordante marked an inline comment as done.
Mordante updated this revision to Diff 493986.
Mordante updated this revision to Diff 494021.
Mordante updated this revision to Diff 494326.
Mordante updated this revision to Diff 494895.
Mordante updated this revision to Diff 494897.
Mordante updated this revision to Diff 495071.
Mordante retitled this revision from "[libc++][test] Removes rapid-cxx-test.h." to "[libc++][test] Adds more generic test macros.".
Mordante edited the summary of this revision.
Mordante published this revision for review.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.
Fixes CI,
Mordante added a comment.
Thanks for the review!
Mordante added a comment.
Addresses review comments.
Mordante added a comment.
CI fixes
Mordante added a comment.
CI fixes.
Mordante added a comment.
Rebased and a bit of polishing.
Mordante added a comment.
Rebased on D143203 <https://reviews.llvm.org/D143203> and added documentation.
Mordante added a comment.
CI fixes.
Mordante added a comment.
Remove fs and unneeded format changes to keep this review to its basics.
================
Comment at: libcxx/test/support/assert_macros.h:56
# endif
return "Message discarded since it can't be streamed to std::cerr.\n";
}
----------------
I would use `abort` instead of `exit` -- some platforms can provide `abort` but can't provide `exit`, because `exit` has to be able to terminate the program successfully, which doesn't make sense on some platforms. `abort()` should be available on more platforms than `exit`.
================
Comment at: libcxx/test/support/assert_macros.h:59
+# define TEST_WRITE_TO_CERR(...) [&] { std::cerr << test_concat_message(__VA_ARGS__); }
+
----------------
It would also be nice to avoid mentioning `std::cerr` here. Would it make sense to pass a file descriptor like `stderr` to the functor being called so it can use `std::fprintf(descriptor, ...)` directly?
================
Comment at: libcxx/test/support/assert_macros.h:59
+# define TEST_WRITE_TO_CERR(...) [&] { std::cerr << test_concat_message(__VA_ARGS__); }
+
----------------
ldionne wrote:
> It would also be nice to avoid mentioning `std::cerr` here. Would it make sense to pass a file descriptor like `stderr` to the functor being called so it can use `std::fprintf(descriptor, ...)` directly?
I don't think it makes sense to provide a descriptor. This should only log messages when an error occurs. Using `printf(stderr, ...)` instead of `std::cerr`, probably makes sense.
================
Comment at: libcxx/test/support/assert_macros.h:85-87
+void test_fail(const char* file, int line, Arg&& arg) {
+ test_log_error("", file, line, std::forward<Arg>(arg));
}
----------------
I would separate the `exit` out of the logging function. It would look like this ^.
================
Comment at: libcxx/test/support/assert_macros.h:90-94
+void test_require(bool condition, const char* condition_str, const char* file, int line, Arg&& arg) {
if (condition)
return;
+ test_log_error(condition_str, file, line, std::forward<Arg>(arg));
----------------
================
Comment at: libcxx/test/support/assert_macros.h:110
// assert(false) replacement
-#define TEST_FAIL(MSG) ::test_fail(__FILE__, __LINE__, MSG)
+// The ARG either a
+// - c-ctring or std::string, in which case the string is printed to stderr,
----------------
================
Comment at: libcxx/test/support/assert_macros.h:125
+// ARG is the same as for TEST_FAIL
+#define TEST_LIBCPP_REQUIRE(CONDITION, ARG) ::test_libcpp_require(CONDITION, #CONDITION, __FILE__, __LINE__, ARG)
+
----------------
Why not define this conditionally to nothing or to `test_require`? That would allow getting rid of `test_libcpp_require` above.
================
Comment at: libcxx/test/support/assert_macros.h:125
+// ARG is the same as for TEST_FAIL
+#define TEST_LIBCPP_REQUIRE(CONDITION, ARG) ::test_libcpp_require(CONDITION, #CONDITION, __FILE__, __LINE__, ARG)
+
----------------
ldionne wrote:
> Why not define this conditionally to nothing or to `test_require`? That would allow getting rid of `test_libcpp_require` above.
Good point!
================
Comment at: libcxx/test/support/assert_macros.h:133
+ } catch (...) { \
+ test_log_error(#EXPR, __FILE__, __LINE__, "no exception was expected\n"); \
+ } \
----------------
================
Comment at: libcxx/test/support/assert_macros.h:141
+ static_cast<void>(EXPR); \
+ test_log_error(nullptr, \
+ __FILE__, \
----------------
Please fully qualify throughout.
================
Comment at: libcxx/test/support/assert_macros.h:155
+
+# define TEST_VALIDATE_EXCEPTION(EXPR, TYPE, PRED) \
+ do { \
----------------
I feel that the filesystem order of arguments is clearer: exception type, exception predicate and finally the expression. Not a super strong opinion though.
================
Comment at: libcxx/test/support/assert_macros.h:163
+ "no exception is thrown while an exception of type " #TYPE " was expected\n"); \
+ } catch (const TYPE& EXCEPTION) { \
+ PRED(EXCEPTION); \
----------------
To avoid using a non-reserved name?
================
Comment at: libcxx/test/support/assert_macros.h:163
+ "no exception is thrown while an exception of type " #TYPE " was expected\n"); \
+ } catch (const TYPE& EXCEPTION) { \
+ PRED(EXCEPTION); \
----------------
ldionne wrote:
> To avoid using a non-reserved name?
I wouldn't mind using `exception`, but we shouldn't use reserved names in our tests. This would break the test when a standard library uses `__exception`.
These macros are intended to replace the macros in rapid-cxx-test.h.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D142808
Files:
libcxx/docs/Contributing.rst
libcxx/docs/TestingLibcxx.rst
libcxx/test/std/containers/container.adaptors/container.adaptors.format/format.functions.format.pass.cpp
libcxx/test/std/containers/container.adaptors/container.adaptors.format/format.functions.vformat.pass.cpp
libcxx/test/std/containers/sequences/vector.bool/vector.bool.fmt/format.functions.format.pass.cpp
libcxx/test/std/containers/sequences/vector.bool/vector.bool.fmt/format.functions.vformat.pass.cpp
libcxx/test/std/utilities/format/format.functions/escaped_output.ascii.pass.cpp
libcxx/test/std/utilities/format/format.functions/escaped_output.unicode.pass.cpp
libcxx/test/std/utilities/format/format.functions/format.locale.pass.cpp
libcxx/test/std/utilities/format/format.functions/format.pass.cpp
libcxx/test/std/utilities/format/format.functions/format_tests.h
libcxx/test/std/utilities/format/format.functions/locale-specific_form.pass.cpp
libcxx/test/std/utilities/format/format.functions/vformat.locale.pass.cpp
libcxx/test/std/utilities/format/format.functions/vformat.pass.cpp
libcxx/test/std/utilities/format/format.range/format.range.fmtmap/format.functions.format.pass.cpp
libcxx/test/std/utilities/format/format.range/format.range.fmtmap/format.functions.vformat.pass.cpp
libcxx/test/std/utilities/format/format.range/format.range.fmtset/format.functions.format.pass.cpp
libcxx/test/std/utilities/format/format.range/format.range.fmtset/format.functions.vformat.pass.cpp
libcxx/test/std/utilities/format/format.range/format.range.formatter/format.functions.format.pass.cpp
libcxx/test/std/utilities/format/format.range/format.range.formatter/format.functions.vformat.pass.cpp
libcxx/test/std/utilities/format/format.tuple/format.functions.format.pass.cpp
libcxx/test/std/utilities/format/format.tuple/format.functions.vformat.pass.cpp
libcxx/test/support/assert_macros.h
libcxx/test/support/concat_macros.h
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D142808.495071.patch
Type: text/x-patch
Size: 48906 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230206/a57c6d91/attachment-0001.bin>
More information about the libcxx-commits
mailing list