[libcxx-commits] [PATCH] D67900: [libc++] Use builtin type traits whenever possible

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter added inline comments.


================
Comment at: libcxx/include/type_traits:562
 _LIBCPP_INLINE_VAR _LIBCPP_CONSTEXPR bool is_same_v
     = is_same<_Tp, _Up>::value;
 #endif
----------------
Duplicate this on both branches of your new conditional, and optimize it on the `__has_keyword(__is_same)` branch? 


================
Comment at: libcxx/include/type_traits:567
 using _IsSame = _BoolConstant<
 #ifdef __clang__
     __is_same(_Tp, _Up)
----------------
It's peculiar that this (and line 576) uses `#ifdef __clang__` vs. your `__has_keyword(__is_same)`. Should the two be consistent? Should `_IsSame` and `_IsNotSame` move up into your new conditional?


================
Comment at: libcxx/include/type_traits:672
 _LIBCPP_INLINE_VAR _LIBCPP_CONSTEXPR bool is_const_v
     = is_const<_Tp>::value;
 #endif
----------------
Ditto "Should the variable template form be optimized as well to avoid the class template instantiation?" (I'll stop marking these, but IMO all of the `is_meow_v` variable templates should use the intrinsics directly rather than unnecessarily instantiate a class template.)


================
Comment at: libcxx/include/type_traits:758
+template <class _Tp>
+struct __libcpp_is_void : _BoolConstant<__is_void(_Tp)> { };
+
----------------
This isn't equivalent: `__is_void(cv-void)` is `true`, but `__libcpp_is_void<cv-void>` is `false`.


================
Comment at: libcxx/include/type_traits:935
+    __has_keyword(is_rvalue_reference) && \
+    __has_keyword(is_reference)
+
----------------
1. Why are these grouped like this, when none of the above are so grouped?
2. Shouldn't `is_rvalue_reference` and `is_reference` have leading double underscores?


================
Comment at: libcxx/include/type_traits:1165
+// Before clang 10, this builtin did not work for nullptr_t
+#if __has_keyword(__is_fundamental) && _LIBCPP_CLANG_VER > 900
+
----------------
Should this be `>= 1000` instead of `> 900` to avoid catching e.g. 9.0.1, or has the bug been fixed on the Clang 9 branch also? (If so, the comment could be clarified.)


================
Comment at: libcxx/include/type_traits:1394
 
 template <class _Tp> using remove_cvref_t = typename remove_cvref<_Tp>::type;
 #endif
----------------
C++20 would greatly appreciate a `__remove_cvref(T)` intrinsic which could be used to implement the above.


================
Comment at: libcxx/include/type_traits:1441
+// Before clang 10, this builtin did not work for floating points or enums
+#if __has_keyword(__is_signed) && _LIBCPP_CLANG_VER > 900
+
----------------
Ditto "Should this actually be `_LIBCPP_CLANG_VER >= 1000`, or should the comment be clarified?"


================
Comment at: libcxx/test/std/utilities/meta/meta.rel/is_same.pass.cpp:62
+		void fn(std::is_same<T, float>) { }
+		void fn(std::false_type) { }
+  	void x() { std::disjunction<std::is_same<T, int>, std::is_same<T, float>, std::false_type>(); }
----------------
Maybe add a `void fn(std::true_type) { }` to ensure that `std::is_same<T, meow>` is not `true_type` even when `T` and `meow` are the same type?


================
Comment at: libcxx/test/std/utilities/meta/meta.rel/is_same.pass.cpp:63
+		void fn(std::false_type) { }
+  	void x() { std::disjunction<std::is_same<T, int>, std::is_same<T, float>, std::false_type>(); }
+};
----------------
1. `std::disjunction` is a C++17 feature.
2. `std::disjunction</* ...stuff... */, std::false_type>` is a different type than, but has the same base characteristic as `std::disjunction</* ...stuff... */>`, so the `false_type` here does nothing.
3. The body of this member function is not instantiated for any specialization of this class template in this test file, so it is extraneous.
4. This member does nothing useful, so there seems to be no need to instantiate it.



================
Comment at: libcxx/test/std/utilities/meta/meta.rel/is_same.pass.cpp:90
+  	OverloadTest<char> t2;
+  	assert(t1.dummy && t2.dummy);
+#endif
----------------
Defining variables suffices to instantiate the class template specializations, so this `assert` seems to serve no useful purpose.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67900