This is an archive of the discontinued LLVM Phabricator instance.

[libc] Let exhaustive tests indicate each interval PASSED/FAILED.
ClosedPublic

Authored by lntue on Mar 13 2022, 8:44 PM.

Details

Summary

Let exhaustive tests indicate each interval PASSED/FAILED.

Diff Detail

Event Timeline

lntue created this revision.Mar 13 2022, 8:44 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 13 2022, 8:44 PM
lntue requested review of this revision.Mar 13 2022, 8:44 PM
sivachandra added inline comments.Mar 13 2022, 11:14 PM
libc/test/src/math/exhaustive/exhaustive_test.h
26

Since its a bool value, would it better as a return value instead of reference argument?

libc/test/src/math/exhaustive/expf_test.cpp
31

If you use EXPECT_<...> macro as opposed to ASSERT_<...> macro, wouldn't result always get set to true?

lntue updated this revision to Diff 415096.Mar 14 2022, 8:06 AM

Let EXPECT_THAT return boolean results.

lntue added inline comments.Mar 14 2022, 8:10 AM
libc/test/src/math/exhaustive/exhaustive_test.h
26

Updated to return to bool value.

libc/test/src/math/exhaustive/expf_test.cpp
31

Fixed EXPECT_<...> macros to return bool values.

sivachandra added inline comments.Mar 15 2022, 2:21 AM
libc/test/src/math/exhaustive/exp2f_test.cpp
31

This pattern seems to have a logic flaw. For example, since we use an EXPECT_<> macro, a following success/failure can overwrite the previous failure/success. You probably want result &&= <...> on line 28?

libc/utils/UnitTest/LibcTest.h
404

To math the pattern of other ASSERT_<> macros, can we construct ASSERT_THAT using EXPECT_THAT?

lntue updated this revision to Diff 415399.Mar 15 2022, 5:57 AM

Address comments.

lntue marked 2 inline comments as done.Mar 15 2022, 6:02 AM
lntue added inline comments.
libc/test/src/math/exhaustive/exp2f_test.cpp
31

Good catch!

libc/utils/UnitTest/LibcTest.h
404

Look like we need to have another patch to wrap other ASSERT_<> in do { ... } while (0) to make it not stealing else clause if being used inside other if statement.

lntue updated this revision to Diff 415426.Mar 15 2022, 7:34 AM
lntue marked 2 inline comments as done.

Sync.

sivachandra accepted this revision.Mar 16 2022, 12:52 AM
sivachandra added inline comments.
libc/utils/UnitTest/LibcTest.h
404

They are not compound statements so shouldn't require a do {...} while (0) treatment?

This revision is now accepted and ready to land.Mar 16 2022, 12:52 AM
lntue added inline comments.Mar 16 2022, 6:55 AM
libc/utils/UnitTest/LibcTest.h
404

What I'm thinking is in the case that we have something like:

if (cond)
  ASSERT_<>(...);
else
  ASSERT_<>(...);

then without the do { ... } while (0) treatment, it will silently do the wrong branching due to the else is stolen from the first ASSERT.
Maybe we should not have that pattern but it might be inevitable sometime.