[libcxx-commits] [PATCH] D131732: [libcxx][NFC] utilises compiler builtins for unary transform type-traits

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D131732#3717939 <https://reviews.llvm.org/D131732#3717939>, @cjdb wrote:

> In D131732#3717669 <https://reviews.llvm.org/D131732#3717669>, @philnik wrote:
>
>> Which compiler supports these builtins? Neither GCC nor clang accept `__add_lvalue_reference` or `__add_pointer`: https://godbolt.org/z/nvxqWTGY1
>
> See D116203 <https://reviews.llvm.org/D116203>.

Could you rebase on top of D116203 <https://reviews.llvm.org/D116203> to run the clang patch with this patch together in the CI? I think it would be good to land these two patches at the same time.
Do you have any benchmarks to show how much of a difference it makes?



================
Comment at: libcxx/include/__type_traits/add_lvalue_reference.h:21
 
-template <class _Tp, bool = __is_referenceable<_Tp>::value> struct __add_lvalue_reference_impl            { typedef _LIBCPP_NODEBUG _Tp  type; };
+#if __has_builtin(__add_lvalue_reference)
+template <class _Tp> struct add_lvalue_reference { typedef _LIBCPP_NODEBUG __add_lvalue_reference(_Tp) type; };
----------------
For all of the changes: Please clang-format them and use `using` instead of `typedef`.


================
Comment at: libcxx/include/__type_traits/decay.h:61
 };
+#endif
 
----------------
Please also add a comment to other longer preprocessor conditions.


================
Comment at: libcxx/include/__type_traits/is_referenceable.h:24
+template <class _Tp>
+struct __referenceable : integral_constant<bool, __is_referenceable(_Tp)> {};
+#else
----------------
I think a better name would be `__libcpp_is_referenceable`. `__referenceable` sounds a lot like a concept, not a type trait.


================
Comment at: libcxx/include/__type_traits/remove_cvref.h:30
+
+template <class _Tp> using remove_cvref_t = typename remove_cvref<_Tp>::type;
+#endif
----------------
This is just a duplicate of line 43. Could you move them out of the `#if __has_builtin()` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131732