Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/test/std/depr/depr.c.headers/math_h.pass.cpp | ||
|---|---|---|
| 833–835 | Oops, yes. They should be TEST_HAS_NO_CHAR8_T. | |
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.
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.
| 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. | |
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>()(); | |
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.
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"