[PATCH] D54472: Disable invalid isPodLike<> specialization

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

Quuxplusone added inline comments.


================
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
----------------
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.


================
Comment at: include/llvm/Analysis/BlockFrequencyInfoImpl.h:188
 
-    BlockNode() = default;
+    BlockNode() noexcept(true) = default;
     BlockNode(IndexType Index) : Index(Index) {}
----------------
serge-sans-paille wrote:
> Quuxplusone wrote:
> > Unintended diff? Also, surely `noexcept(true)` a.k.a. `noexcept` would be the default here anyway. If anything wants `noexcept`, it'd be line 189, not line 188. :)
> That's unfortunate but intended: for some reason, and for some clang version, the type trait ``is_trivially_copyable`` leads to this being required. There's no such issue with the gcc version I compiled clang with.
That's extremely weird and scary. Is it worth digging further into why that happens?


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

https://reviews.llvm.org/D54472