[libcxx-commits] [PATCH] D96657: [libcxx] adds common_reference to <type_traits>

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb marked an inline comment as done.
cjdb added inline comments.


================
Comment at: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp:10
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17
+
----------------
Mordante wrote:
> Make the test unsupported if concepts support is lacking.
Thanks, this one was really driving me round the twist.


================
Comment at: libcxx/test/std/utilities/meta/meta.trans/meta.trans.other/common_reference.compile.pass.cpp:22-28
+template <class>
+constexpr bool is_trait = false;
+template <class T>
+requires requires {
+  typename T::type;
+}
+constexpr bool is_trait<T> = true;
----------------
rarutyun wrote:
> I think you don't need the specialization of template variable. Since `requires` expression is already `constexpr bool` semantically, you may use it directly to initialize `constexpr bool` variable and you don't need `requires`-clause
> 
> ```
> template <class T>
> constexpr bool is_trait = requires { typename T::type; };
> ```
> 
> Proof: https://godbolt.org/z/fjjqeE. Please see the program output for two different compilers depending on defined macro
> 
> Small naming suggestion. I would rename `is_trait` to `has_type`. It explicitly says what you check without looking deeply into the logic how it's determined. But it's up to you. And I would add `_v` suffix (nevermind if you rename it or not) to just emphasize that it's value rather than the type
> I think you don't need the specialization of template variable. Since requires expression is already constexpr bool semantically, you may use it directly to initialize constexpr bool variable and you don't need requires-clause

Good point! Done.

> Small naming suggestion. I would rename is_trait to has_type. It explicitly says what you check without looking deeply into the logic how it's determined. But it's up to you.

Agreed, done.

> And I would add _v suffix (nevermind if you rename it or not) to just emphasize that it's value rather than the type

The name already does this in both cases, so I don't think it's necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96657