This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Adds new test macros.
ClosedPublic

Authored by Mordante on Dec 24 2022, 5:09 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGf8bed1369421: [libc++][format] Adds new test macros.
Summary

These macros make it easier to log additional information. This is
useful for formatting tests. It also properly disables additional
information when locales are disabled in libc++.

Diff Detail

Event Timeline

Mordante created this revision.Dec 24 2022, 5:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 24 2022, 5:09 AM
Mordante requested review of this revision.Dec 24 2022, 5:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 24 2022, 5:09 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Jan 10 2023, 9:27 AM
ldionne added a subscriber: ldionne.

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).

libcxx/test/support/test_validation.h
1 ↗(On Diff #485196)

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.

34 ↗(On Diff #485196)
41 ↗(On Diff #485196)
51 ↗(On Diff #485196)

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.

53 ↗(On Diff #485196)

Here and elsewhere.

63 ↗(On Diff #485196)

I would make those normal function templates instead. Otherwise one wonders whether there's a reason to define them this way.

84 ↗(On Diff #485196)
88 ↗(On Diff #485196)

All of these macros should fully qualify what they call.

91 ↗(On Diff #485196)

I think I would name the macros this way:

TEST_FAIL(...)
TEST_ASSERT(...)
TEST_LIBCPP_ASSERT(...)
This revision now requires changes to proceed.Jan 10 2023, 9:27 AM
Mordante marked 9 inline comments as done.Jan 10 2023, 11:29 AM
Mordante added inline comments.
libcxx/test/support/test_validation.h
51 ↗(On Diff #485196)

As discussed I've done so. This still needs an additional include to "stringify" the messages.

Mordante updated this revision to Diff 487906.Jan 10 2023, 11:32 AM
Mordante marked an inline comment as done.

Rebased and addresses review comments.

Mordante updated this revision to Diff 487913.Jan 10 2023, 11:39 AM

Forgot to save the last buffer which breaks the CI.

Mordante updated this revision to Diff 489019.Jan 13 2023, 8:04 AM

Rebased to test CI.

Mordante updated this revision to Diff 489038.Jan 13 2023, 9:19 AM

Fixes CI.

ldionne added inline comments.Jan 17 2023, 8:47 AM
libcxx/test/support/assert_macros.h
2

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.

Mordante added inline comments.Jan 17 2023, 9:57 AM
libcxx/test/support/assert_macros.h
2

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.

ldionne accepted this revision.Jan 17 2023, 10:11 AM
ldionne added inline comments.
libcxx/test/support/assert_macros.h
2

SGTM.

This revision is now accepted and ready to land.Jan 17 2023, 10:11 AM
This revision was landed with ongoing or failed builds.Jan 18 2023, 8:01 AM
This revision was automatically updated to reflect the committed changes.