This is an archive of the discontinued LLVM Phabricator instance.

[libc++][test] Mark `test_comparisons.h` helpers as nodiscard
ClosedPublic

Authored by avogelsgesang on Aug 7 2022, 10:36 AM.

Details

Summary

I accidentally wrote testComparisons(...) instead of
assert(testComparisons(...). This compiled without issues, but
did not provide the intended test coverage. By adding a nodiscard,
we can make sure that the compiler catches such mistakes for us.

Diff Detail

Event Timeline

avogelsgesang created this revision.Aug 7 2022, 10:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2022, 10:36 AM
avogelsgesang requested review of this revision.Aug 7 2022, 10:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2022, 10:36 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mumbleskates accepted this revision.Aug 10 2022, 5:33 PM

Thanks! looks good to me

philnik requested changes to this revision.Aug 11 2022, 1:17 AM

You shouldn't use libc++-specific macros in the tests. Instead, add a new macro to test_macros.h and use it here. Otherwise the tests that use test_comparisons.h would break for other implementations.

This revision now requires changes to proceed.Aug 11 2022, 1:17 AM

use TEST_NODISCARD instead of _LIBCPP_NODISCARD

philnik accepted this revision.Aug 11 2022, 4:21 PM

LGTM with green CI. You missed a ) for the assert in the description.

This revision is now accepted and ready to land.Aug 11 2022, 4:21 PM