This is an archive of the discontinued LLVM Phabricator instance.

[libc] Enable custom logging in LibcTest
ClosedPublic

Authored by gchatelet on Jun 10 2023, 1:36 PM.

Details

Summary

This patch mimics the behavior of Google Test and allow users to log custom messages after all flavors of ASSERT_ / EXPECT_.

Diff Detail

Event Timeline

gchatelet created this revision.Jun 10 2023, 1:36 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 10 2023, 1:36 PM
gchatelet requested review of this revision.Jun 10 2023, 1:36 PM
gchatelet updated this revision to Diff 530249.Jun 10 2023, 1:59 PM
  • Simplify implementation and add documentation
lntue added inline comments.Jun 13 2023, 6:31 AM
libc/test/UnitTest/LibcTest.h
53

Do you know why the Matcher's interface cannot be const T&? Ideally it should not modify the inputs, right?

gchatelet updated this revision to Diff 530893.Jun 13 2023, 6:47 AM
gchatelet marked an inline comment as done.
  • match should take a 'const T&' instead of a 'T&'
libc/test/UnitTest/LibcTest.h
53

I traced it down to https://reviews.llvm.org/D75487
I don't see a rationale for this choice so I assume it was just overlooked.

I refrained from fixing it in this patch to prevent noise but since you're mentioning it, here it is.

lntue accepted this revision.Jun 13 2023, 8:04 AM
This revision is now accepted and ready to land.Jun 13 2023, 8:04 AM
sivachandra accepted this revision.Jun 14 2023, 12:30 AM

LGTM with one question.

libc/test/UnitTest/TestLogger.cpp
72

Why should we use [u]int<..>_t types? Could it lead to duplicate declaration errors, for example if int64_t == long long?

gchatelet updated this revision to Diff 531236.Jun 14 2023, 2:18 AM
gchatelet marked an inline comment as done.
  • rebase
  • Simplify implementation and add documentation
  • match should take a 'const T&' instead of a 'T&'
  • Use standard C types for TesLogger specializaions
libc/test/UnitTest/TestLogger.cpp
72

You're right, thx for catching it. I've used the standard C types.

This revision was automatically updated to reflect the committed changes.
gchatelet reopened this revision.Jun 14 2023, 4:50 AM

Build bots were failing because of a few other bool r = EXPECT_FP_EQ(... and bool r = EXPECT_MPFR_MATCH_ROUNDING(... that I didn't catch.
They were only built when LLVM_LIBC_FULL_BUILD was not defined.

This revision is now accepted and ready to land.Jun 14 2023, 4:50 AM
gchatelet updated this revision to Diff 531265.Jun 14 2023, 5:01 AM
  • Fix remaining math tests
gchatelet added a comment.EditedJun 14 2023, 5:03 AM

@lntue can you have another look?
Changed lines from last time you gave me LGTM : https://reviews.llvm.org/D152630?vs=531248&id=531265#toc

We should probably submit these math related changes as a separate patch BTW

lntue accepted this revision.Jun 14 2023, 6:11 AM
This revision was automatically updated to reflect the committed changes.