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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/include/type_traits:567
 using _IsSame = _BoolConstant<
 #ifdef __clang__
     __is_same(_Tp, _Up)
----------------
CaseyCarter wrote:
> 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).
Clang 3.5.0 is the latest supported version so, that should be fine. 


================
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:
> 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.
I can't use braces because of C++03 mode. I fixed it by casting to void, thanks for the suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67900