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

Tue Jan 17 10:11:54 PST 2023

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.

================
Comment at: libcxx/test/support/assert_macros.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
Mordante wrote:
> 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.
SGTM.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140651