This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove duplication in math_h.pass.cpp and improve coverage
ClosedPublic

Authored by philnik on Oct 27 2022, 6:12 PM.

Diff Detail

Event Timeline

philnik created this revision.Oct 27 2022, 6:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 6:12 PM
philnik requested review of this revision.Oct 27 2022, 6:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 6:12 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
huixie90 accepted this revision as: huixie90.Oct 28 2022, 12:48 AM

LGTM with nits

libcxx/test/std/depr/depr.c.headers/math_h.pass.cpp
833–835

Slightly confused by the condition. I read it as "If char8_t is NOT defined, then we have the test to test the char8_t"

918

same here

981

same here

philnik updated this revision to Diff 471532.Oct 28 2022, 6:44 AM
philnik marked 3 inline comments as done.

Address comments

philnik added inline comments.Oct 29 2022, 1:43 PM
libcxx/test/std/depr/depr.c.headers/math_h.pass.cpp
833–835

Oops, yes. They should be TEST_HAS_NO_CHAR8_T.

philnik updated this revision to Diff 471782.Oct 29 2022, 1:55 PM

Try to fix CI

philnik updated this revision to Diff 471841.Oct 30 2022, 9:48 AM

Also instantiate all the functions

EricWF requested changes to this revision.Oct 30 2022, 10:02 AM
EricWF added a subscriber: EricWF.

Missing description.

All changes need a description.

In particular, why is the behavior we're adding for int128 correct, and why do we want it?

Further, I don't think deduplicated these tests is useful. It makes the tests harder to read by virtue of introducing indirection.
I don't think this change should proceed. I don't think it's an improvement.

This revision now requires changes to proceed.Oct 30 2022, 10:02 AM

Missing description.

All changes need a description.

What description do you want? "It improves coverage." or what?

In particular, why is the behavior we're adding for int128 correct, and why do we want it?

I'm not sure what you're asking about with "why the behaviour is correct". We claim support for __int128, so I consider it to be a bug.

Further, I don't think deduplicated these tests is useful. It makes the tests harder to read by virtue of introducing indirection.

It's not just deduplicating. We are currently lacking coverage for a lot of types, which this patch adds. It is harder to check which combinations of types are checked, but with the indirection you only have to check it once, not for every function. I'm quite certain the duplication was a contributing factor why we didn't have any coverage for most types.

ldionne requested changes to this revision.Nov 3 2022, 8:43 AM
ldionne added inline comments.
libcxx/test/std/depr/depr.c.headers/math_h.pass.cpp
879

Same here, move at the top.

978

Could you investigate whether adding a separate function that does the cartesian product (and only that) would simplify the test a lot? Right now I find it quite difficult to follow from test_integral_single_arg to test_integral_two_args and then test_two_args, etc. I suspect exposing the cartesian produce separately would make it easier to follow.

1007

Can you move this at the top of the function? IMO it makes more sense to call this one first, and then do the cartesian product.

Also, I whenever you are passing a result type, I would say /*Result=*/, it helps understand what's going on.

This revision now requires changes to proceed.Nov 3 2022, 8:43 AM
philnik updated this revision to Diff 473436.Nov 5 2022, 9:07 AM
philnik marked an inline comment as done.

Address comments

philnik updated this revision to Diff 473459.Nov 5 2022, 1:47 PM
  • Rebased
  • Try to fix CI
EricWF requested changes to this revision.Nov 6 2022, 2:42 PM

Stop being clever in tests. These changes add a substantial cognitive load to any person who's trying to read them for the first time and understand what they do.

This code duplication doesn't cost us anything, but it gives us a lot.

This revision now requires changes to proceed.Nov 6 2022, 2:42 PM

Stop being clever in tests. These changes add a substantial cognitive load to any person who's trying to read them for the first time and understand what they do.

This code duplication doesn't cost us anything, but it gives us a lot.

@philnik is increasing the coverage of this test. It would be better to instead suggest alternative ways to achieve this coverage without being "clever", or to argue for why we don't need additional coverage. I'm sure @philnik isn't being "clever" on purpose -- I've spent quite a bit of time looking at this and I am not sure how to achieve the same effects without introducing some machinery. Manually stamping these instantiations out is simply not realistic, we have something like at most 16 * 16 * 16 = 4096 combinations to test for the three argument functions.

libcxx/test/std/depr/depr.c.headers/math_h.pass.cpp
935–941

Then below you can call:

meta::for_each(meta::integral_types(), test_single_arg</* PromoteResult */ double>());
test_single_arg</* PromoteResult */ float, /* Arg */ float>()();
test_single_arg</* PromoteResult */ double, /* Arg */ double>()();
test_single_arg</* PromoteResult */ long double, /* Arg */ long double>()();
philnik updated this revision to Diff 476969.Nov 21 2022, 12:12 PM
philnik marked an inline comment as done.

Rebased

ldionne accepted this revision.Nov 21 2022, 2:22 PM

This is significantly easier to understand than the previous iterations of this patch. It's still not perfect, but I think this is the best we'll get given that this test needs to support C++03.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 22 2022, 11:36 AM
This revision was automatically updated to reflect the committed changes.