[libcxx-commits] [PATCH] D78427: [libcxx] Add c++20 <numbers>

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius added inline comments.


================
Comment at: libcxx/include/numbers:80
+template<class>
+_LIBCPP_INLINE_VAR _LIBCPP_CONSTEXPR
+                        bool __value = false;
----------------
Given that it's a C++20 addition, I think that you don't need to use `_LIBCPP_CONSTEXPR` and directly write `constexpr`. This remarks concerns other usages below.


================
Comment at: libcxx/include/numbers:81
+_LIBCPP_INLINE_VAR _LIBCPP_CONSTEXPR
+                        bool __value = false;
+
----------------
It would be more readable to call it `__false_value` or alike, because `__value` seems too generic.


================
Comment at: libcxx/include/numbers:85
+constexpr T _AlwaysFalse() {
+   static_assert(__value<T>, "Require Floating Point types");
+   return T();
----------------
Why Using Capital Letters?

What about unifying a bit static assert messages to match other in libc++?

Examples:
```
static_assert(is_trivially_copyable<_Tp>::value, "std::atomic<Tp> requires that 'Tp' be a trivially copyable type");
static_assert(__is_cpp17_move_insertable<allocator_type>::value, "The specified type does not meet the requirements of Cpp17MoveInsertible");
static_assert(__bitop_unsigned_integer<_Tp>::value, "__rotl requires unsigned");
```

Maybe something like "<numbers> header requires a floating-point type" or mentioning the corresponding concept (floating_point)?


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:596
+   "values": {
+     "c++2a": 201907L,
+   },
----------------
It should be written as `int(201907)`, otherwise it won't work on Python3.


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

https://reviews.llvm.org/D78427