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

[Casey Carter via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/include/type_traits:567
 using _IsSame = _BoolConstant<
 #ifdef __clang__
     __is_same(_Tp, _Up)
----------------
zoecarver wrote:
> 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. 
Some Compiler Explorer research (https://godbolt.org/z/u1JVsC) shows that `__is_same` is supported at least back to clang 3.0.0, but `__is_identifier` (which is used to implement `__has_keyword`) was implemented in 3.5. Consequently there's a range of versions that will use `__is_same` here but not above, which is weird. It seems to me that line 547 should simply check `#ifdef __clang__` since the library does not support clang versions that don't have `__is_same` (if any exist).


================
Comment at: libcxx/include/type_traits:1394
 
 template <class _Tp> using remove_cvref_t = typename remove_cvref<_Tp>::type;
 #endif
----------------
zoecarver wrote:
> CaseyCarter wrote:
> > C++20 would greatly appreciate a `__remove_cvref(T)` intrinsic which could be used to implement the above.
> See D67052 and D67588 :) 
I suppose the extra few nanoseconds that `__remove_cv(__remove_reference(T))` costs over a `__remove_cvref(T)` is negligible. It's certainly in the noise compared to this alias that instantiates a class that instantiates another class that instantiates *two* classes. ;)


================
Comment at: libcxx/test/std/utilities/meta/meta.rel/is_same.pass.cpp:90
+  	OverloadTest<char> t2;
+  	assert(t1.dummy && t2.dummy);
+#endif
----------------
zoecarver wrote:
> 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.
I believe it's conventional in the test suite to silence unused variable warnings by casting to `void`, e.g., `(void) t1;`. In this particular case, however, you don't need the names for anything: `OverloadTest` only needs to be instantiated to achieve its purpose. You could simply create temporaries instead of named variables a la `OverloadTest<int>{}; OverloadTest<char>{};` which avoids defining any variables in the first place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67900