[libcxx-commits] [PATCH] D115100: [libc++] Enable the optimized _IsSame on GCC as well as Clang.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/tuple:1120
+template <class ..._Ts>
+struct __all_are_swappable : __all<__is_swappable<_Ts>::value...> {};
+
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Instead, I really feel like we could redefine `__all` to be:
> > 
> > ```
> > template <bool ..._Pred>
> > struct __all : _IsSame<__all_dummy<_Pred...>, __all_dummy<((void)_Pred, true)...>> { };
> > ```
> > 
> > After all, we will generally instantiate `__all` with several types, however we probably won't instantiate `__all` itself several times. What I'm trying to say is that since `__all` is variadic, we will probably instantiate it some factor less than the number of types that we're passing into it, so it might be reasonable to define it as a `struct` despite the additional costs. That would fix our mangling problem without needing a definition for `__all_are_swappable`, which IMO is kind of weird.
> That would fix the mangling problem for `__all` specifically, but that's an O(n) patch for an O(n^2) problem (doesn't fix things that aren't `__all`), whereas what I've done here is the O(1) patch for the O(n^2) problem.
> Personally I'd rather keep the O(1) paper-over patch for now, and then if we see this Clang/GCC incompatibility issue coming up repeatedly, we think about how to generalize it //then//. I suspect the issue won't come up much at all, because C++>=20 stuff will all use `requires` instead of return-type SFINAE.
The problem I have with fixing it locally is that we'll eventually hit the same problem if we use `__all` in another place. It seems cleaner to me to fix `__all` and add a comment explaining the GCC bug. Then, file a bug with GCC and reference it in the comment. That way, we pessimize `__all` but we reduce the likelihood of hitting the bug again to the maximum, and eventually we'll remove the pessimization when the GCC bug is fixed.


================
Comment at: libcxx/include/type_traits:553
 
-#if __has_keyword(__is_same)
+#if __has_builtin(__is_same)
 
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Do we support any compilers where this is false? I don't think so -- we could probably just drop the `#if` altogether.
> It appeared to me that MSVC doesn't support `__has_builtin` at all, so it ends up nerfed to false by default.
> However, I'm OK with dropping the `#if` and seeing what happens. Someone can always put it back later.
AFAICT we have CI for all our supported platforms. If CI is happy without `#if`, I think that means we're good. If not, then let's keep the `#if`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115100