[libcxx-commits] [PATCH] D136908: [libc++] Remove duplication in math_h.pass.cpp and improve coverage

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/std/depr/depr.c.headers/math_h.pass.cpp:878
+
+  test_two_args<T, U, PromoteResult>();
+}
----------------
Same here, move at the top.


================
Comment at: libcxx/test/std/depr/depr.c.headers/math_h.pass.cpp:977
+void test_integral_single_arg() {
+  test_integral_two_args<T, bool>();
+  test_integral_two_args<T, char>();
----------------
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.


================
Comment at: libcxx/test/std/depr/depr.c.headers/math_h.pass.cpp:1006
+  test_integral_two_args<T, long double, /*Result=*/long double>();
+  test_single_arg<T, double>();
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136908/new/

https://reviews.llvm.org/D136908