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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 9 inline comments as done.
zoecarver 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
----------------
CaseyCarter wrote:
> Duplicate this on both branches of your new conditional, and optimize it on the `__has_keyword(__is_same)` branch? 
I am planning on doing this but, I am going to do it in another patch. 


================
Comment at: libcxx/include/type_traits:567
 using _IsSame = _BoolConstant<
 #ifdef __clang__
     __is_same(_Tp, _Up)
----------------
CaseyCarter wrote:
> 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?
IIRC clang has always had `__is_same` whereas that is not true for all these traits. I wasn't always sure (and I didn't want to write a lot of "random" version `#if`s) so, to be safe, I looked for the keyword. It might be good to check for `__clang__` //as well//, though. 


================
Comment at: libcxx/include/type_traits:672
 _LIBCPP_INLINE_VAR _LIBCPP_CONSTEXPR bool is_const_v
     = is_const<_Tp>::value;
 #endif
----------------
CaseyCarter wrote:
> 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.)
Agreed. See my comment above.


================
Comment at: libcxx/include/type_traits:758
+template <class _Tp>
+struct __libcpp_is_void : _BoolConstant<__is_void(_Tp)> { };
+
----------------
CaseyCarter wrote:
> This isn't equivalent: `__is_void(cv-void)` is `true`, but `__libcpp_is_void<cv-void>` is `false`.
I'll look into this and submit a patch, thanks.


================
Comment at: libcxx/include/type_traits:935
+    __has_keyword(is_rvalue_reference) && \
+    __has_keyword(is_reference)
+
----------------
CaseyCarter wrote:
> 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?
1. I assumed that if one of them was defined then, they all would be defined. 
2. Yep, I'll fix it.


================
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
+
----------------
CaseyCarter wrote:
> 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.)
When clang 9.0.1 branched? I submitted it a few weeks ago. I'll see if I can figure out what version this is in an update the condition/comment.


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


================
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>(); }
+};
----------------
CaseyCarter wrote:
> 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.
> 
Thanks for the notes. I wrote this test a bit hastily. I'll fix it up.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67900