[libcxx-commits] [PATCH] D88131: adds [concepts.arithmetic]

[Louis Dionne via Phabricator via libcxx-commits]

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

Mostly nitpicks, except the part about intrinsics which I would really like to de-duplicate.



================
Comment at: libcxx/include/concepts:180
+// [concepts.arithmetic], arithmetic concepts
+#ifdef __clang__
+template<class _Tp>
----------------
Same comment about not duplicating the implementation to use intrinsics, since they are already used by the type traits.


================
Comment at: libcxx/test/std/concepts/lang/arithmetic.pass.cpp:165
+  static_assert(!CheckSignedIntegralQualifiers<__uint128_t>());
+#endif // _LIBCPP_HAS_NO_INT128
+
----------------
No need for these comments for small `#if` pairs.


================
Comment at: libcxx/test/std/concepts/lang/arithmetic.pass.cpp:264
+    requires std::signed_integral<I> && (!std::unsigned_integral<I>)
+                                            [[nodiscard]] constexpr
+    bool CheckSubsumption(I) {
----------------
Is that the result of clang-format? This seems weirdly aligned, no?


================
Comment at: libcxx/test/std/concepts/lang/arithmetic.pass.cpp:30
+  // conventional integral types
+  static_assert(std::integral<signed char>);
+  static_assert(std::integral<unsigned char>);
----------------
CaseyCarter wrote:
> miscco wrote:
> > Would it simplify the testing when we had a function like this
> > ```cpp
> > template <typename T, bool Expected>
> > constexpr void CheckQualifiers() {
> >      static_assert(std::integral<T> == Expected);
> >      static_assert(std::integral<const T> == Expected);
> >      static_assert(std::integral<volatile T> == Expected);
> >      static_assert(std::integral<const volatile T> == Expected);
> > 
> >      static_assert(!std::integral<T&>);
> >      static_assert(!std::integral<const T&>);
> >      static_assert(!std::integral<volatile T&>);
> >      static_assert(!std::integral<const volatile T&>);
> > 
> >      static_assert(!std::integral<T*>);
> >      static_assert(!std::integral<const T*>);
> >      static_assert(!std::integral<volatile T*>);
> >      static_assert(!std::integral<const volatile T*>);
> > }
> > ```
> Having written a few such functions, I suggest a slight modification to:
> ```
> template <typename T>
> constexpr bool CheckQualifiers() {
>     constexpr bool result = std::integral<T>;
>     static_assert(std::integral<const T> == result);
>     static_assert(std::integral<volatile T> == result);
>     static_assert(std::integral<const volatile T> == result);
> 
>     static_assert(!std::integral<T&>);
>     static_assert(!std::integral<const T&>);
>     static_assert(!std::integral<volatile T&>);
>     static_assert(!std::integral<const volatile T&>);
> 
>     static_assert(!std::integral<T*>);
>     static_assert(!std::integral<const T*>);
>     static_assert(!std::integral<volatile T*>);
>     static_assert(!std::integral<const volatile T*>);
> 
>     return result;
> }
> ```
> so you can more easily `static_assert(CheckQualifiers<int>()); static_assert(!CheckQualifiers<void(*)(int) const>);`.
Really clever.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88131