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

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter added inline comments.


================
Comment at: libcxx/include/concepts:164
 concept derived_from =
   __is_base_of(_Bp, _Dp) && __is_convertible_to(const volatile _Dp*, const volatile _Bp*);
 
----------------
Pre-existing: did GCC start supporting `__is_convertible_to` while I wasn't looking, or is this clang-only?


================
Comment at: libcxx/test/std/concepts/lang/arithmetic.pass.cpp:29
+constexpr void CheckIntegral() {
+  // conventional integral types
+  static_assert(std::integral<signed char>);
----------------
miscco wrote:
> cjdb wrote:
> > This formatting feels off but it's what `git clang-format-12 HEAD~1` gave me. Can someone please confirm this is the correct style?
> I believe that there is no libc++ code style yet, @ldionne what are the chances of defining one?
I'd call these "standard signed and unsigned integer types".


================
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>);
----------------
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>);`.


================
Comment at: libcxx/test/std/concepts/lang/arithmetic.pass.cpp:44
+
+  // integrals that mightn't necessarily be expected as integral
+  static_assert(std::integral<wchar_t>);
----------------
miscco wrote:
> I believe this counts as extended integer types?
"Extended integer types" are implementation-defined types with the same properties as integral types; `__int128` is almost an extended integer type, for example. Formally, these are "integer types that are neither signed nor unsigned integer types" - which is far from a fantastic name. The colloquial "character or bool types" is probably clearer to folks who don't list Standardese as their first language.


================
Comment at: libcxx/test/std/concepts/lang/arithmetic.pass.cpp:75-77
+  struct WithMember {
+    int Sneaky;
+  };
----------------
None of these `Sneaky` members are used - they could be elided. Of course it's then odd to have a dozen-ish empty `struct`s with different names; you could as well use `EmptyStruct` for all of these. (A type `T` doesn't need to have any members of type `U` for us to form the pointer-to-member type `U T::*`.)


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D88131