[PATCH] D54472: Disable invalid isPodLike<> specialization

[Arthur O'Dwyer via Phabricator via llvm-commits]

Quuxplusone added a comment.

LGTM at this point modulo my two comments (one important but trivial, the other completely trivial).
I don't know whether Chandler's concern about `__has_feature` in "include/llvm/ADT/PointerIntPair.h" has been 100% resolved yet — @chandlerc?



================
Comment at: include/llvm/ADT/PointerIntPair.h:133-134
+  static constexpr bool value = true;
+#if !defined(NDEBUG) && (__has_feature(is_trivially_copyable) || (defined(__GNUC__) && __GNUC__ >= 5))
+  static_assert(std::is_trivially_copyable<T>::value, "consistent behavior");
+#endif
----------------
Quuxplusone wrote:
> serge-sans-paille wrote:
> > chandlerc wrote:
> > > The compiler may pass `__has_feature(is_trivially_copyable)` and yet the standard library not provide `std::is_trivially_copyable`. And I don't see any feature test spelled `is_trivially_copyable`? Do you mean `__has_trivially_copy`, which is the Clang and GCC builtin?
> > > 
> > > Also, the message is printed on an *error*.
> > The legacy code for `isPodLike` was using the very same guard without issue, so I tend to think it's okay. +1 for the error stuff, I'll update that.
> I see Clang providing both `__has_feature(has_trivial_copy)` and `__has_feature(is_trivially_copyable)` (using exactly the spellings I just wrote).
> GCC doesn't support `__has_feature` at all, so a bunch of places in LLVM do `#define __has_feature(x) 0`.
> I think this snippet is OK as written.
Wait, you definitely need something like

    using T = PointerIntPair<PointerTy, IntBits, IntType, PtrTraits, Info>;

before this line, or else it won't know what `T` is here! (Probably means you should find a way to compile without `NDEBUG` before merging. :))


================
Comment at: tools/llvm-xray/trie-node.h:45
 
+
 /// Merges together two TrieNodes with like function ids, aggregating their
----------------
Extraneous diff.


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

https://reviews.llvm.org/D54472