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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/test/support/assert_macros.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
ldionne wrote:
> We have something different but similar in `libcxx/test/support/rapid-cxx-test.h`. Have you seen this before? I think it would be nice to unify both.
> 
> I am not a huge fan of having to use a macro for declaring test cases like we do in `rapid-cxx-test.h`, however we do already have macros with the same names. I guess my ask here would be to try and avoid introducing a third way of doing assertions in the test suite:
> - standard `<cassert>`
> - `rapid-cxx-test.h`
> - now this header
> 
> I'd be OK with removing `rapid-cxx-test` if we can replace its uses by this, or I'd be fine with using `rapid-cxx-test.h` in the format tests instead. Sorry, I should have thought of `rapid-cxx-test.h` earlier.
As discussed I wasn't aware of `rapid-cxx-test`. I had a look and I don't see a way for adding custom messages, which is something I really like to use to make the format errors usable.

So my preference would be to remove `rapid-cxx-test`, if you agree I'll add it to my todo list and pick it up after landing the range patches for LLVM 16.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140651