[PATCH] D54472: Disable invalid isPodLike<> specialization

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith added inline comments.


================
Comment at: include/llvm/ADT/PointerIntPair.h:132
+  static constexpr bool value = true;
+#if !defined(NDEBUG) && (__has_feature(is_trivially_copyable) || (defined(__GNUC__) && __GNUC__ >= 5))
+  static_assert(std::is_trivially_copyable<PointerIntPair<PointerTy, IntBits, IntType, PtrTraits, Info>>::value, "inconsistent behavior between llvm:: and std:: is_trivially_copyable");
----------------
serge-sans-paille wrote:
> rsmith wrote:
> > Why is this guarded by `NDEBUG`?
> To limit compile-time overhead.
I don't think that's worth the loss of safety, particularly for folks who aren't clang developers and just try to make a release build with an untested toolchain. I would prefer to take the (likely tiny) compile time cost and always have the check enabled.


================
Comment at: include/llvm/ADT/PointerIntPair.h:133
+#if !defined(NDEBUG) && (__has_feature(is_trivially_copyable) || (defined(__GNUC__) && __GNUC__ >= 5))
+  static_assert(std::is_trivially_copyable<PointerIntPair<PointerTy, IntBits, IntType, PtrTraits, Info>>::value, "inconsistent behavior between llvm:: and std:: is_trivially_copyable");
+#endif
----------------
serge-sans-paille wrote:
> rsmith wrote:
> > I am confused by this. If the type in question is incomplete, how can this `static_assert` work? (And if it's complete, why do you need the specialization at all?) Have you made sure this actually compiles in a Debug build?
> > 
> > If you need this, I think what you should probably do instead is to put this `static_assert` inside `PointerIntPair`, in some member function that is commonly instantiated, such as the default constructor. (It can't go in the body of the class itself because the class isn't complete there.)
> >  If the type in question is incomplete, how can this static_assert work?
> 
> My *guess* is that `std::is_trivially_copyable` 
> 
> > Have you made sure this actually compiles in a Debug build?
> 
> yes. Double checked right now!
Some of your comment seems to have gotten lost here. I still don't see how this can be right (if the type is complete, then this specialization should be unnecessary, and if it's incomplete, then this use of std::is_trivially_copyable should be ill-formed). Have you tried removing this specialization entirely to see if it's actually necessary? If so, what compile error do you get?


================
Comment at: include/llvm/Support/type_traits.h:168
+
+#if !defined(NDEBUG) && (__has_feature(is_trivially_copyable) || (defined(__GNUC__) && __GNUC__ >= 5))
+  static_assert(value == std::is_trivially_copyable<T>::value, "inconsistent behavior between llvm:: and std:: implementation of is_trivially_copyable");
----------------
serge-sans-paille wrote:
> rsmith wrote:
> > Why check `NDEBUG` here?
> It's debug code, to ensure the consistency of `llvm::is_trivially_copyable` with `std::` version. I assume ignoring this on release build make them faster.
(As for the other case, I would prefer that we always `static_assert` this, even in release builds.)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54472