[libcxx-commits] [PATCH] D136538: [libc++] Add test for checking progress on P0533R9

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.

Thanks for working on this! Looks good in general, but I've got a few suggestions.



================
Comment at: libcxx/test/std/numerics/c.math/constexpr.cmath.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
I think this test should be in `libcxx/test/libcxx`, since we don't actually test the standard, but our current implementation status.


================
Comment at: libcxx/test/std/numerics/c.math/constexpr.cmath.pass.cpp:2
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
----------------
Could you also add a test for GCC and the global namespace versions of the functions?


================
Comment at: libcxx/test/std/numerics/c.math/constexpr.cmath.pass.cpp:13
+
+#include "test_macros.h"
+
----------------
What do you need this include for?


================
Comment at: libcxx/test/std/numerics/c.math/constexpr.cmath.pass.cpp:16
+void test_p0533r9() {
+#if TEST_STD_VER >= 23 && defined(__clang__)
+  bool ImplementedP0533R9 = true;
----------------
You can mark the test as `// REQUIRES: clang` and `// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20` instead.


================
Comment at: libcxx/test/std/numerics/c.math/constexpr.cmath.pass.cpp:19
+
+#  define ASSERT_CONSTEXPR_CXX23(Expr) static_assert(__builtin_constant_p(Expr) && (Expr))
+#  define ASSERT_NOT_CONSTEXPR_CXX23(Expr)                                                                             \
----------------
What does `_builtin_constant_p` do?


================
Comment at: libcxx/test/std/numerics/c.math/constexpr.cmath.pass.cpp:30
+
+  ASSERT_NOT_CONSTEXPR_CXX23(std::abs(-1) == 1);       // int abs(int j)
+  ASSERT_NOT_CONSTEXPR_CXX23(std::abs(-1L) == 1L);     // long int abs(long int j)
----------------
I'm not sure these comments add a lot of value, since it's pretty clear what is tested by looking at the call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136538