[PATCH] D54472: Disable invalid isPodLike<> specialization

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a reviewer: rsmith.
chandlerc added a comment.

Given the ... very subtle ABI implications here, I really want Richard to look at this and make sure we're not barking up the wrong tree.

Also I think the example raised in https://godbolt.org/z/59_R-J is a bug in all but MSVC, although I'm not the expert here and Richard may have DRs or issues to cite. My reading of the standard supports the implementation of is_trivially_copyable in this patch and in MSVC. Specifically, [class.prop]p1.2 in the latest draft has the *exact* requirement that this code enforces: one of the copy or move constructors or assignment operators must be non-deleted. And the example given has all of those deleted (all but one because they are defined as deleted implicitly IIRC). But again, happy to defer to Richard here, this is a subtle and complex part of the standard.



================
Comment at: include/llvm/ADT/PointerIntPair.h:128-129
 
+// We need this specialisation to avoid incomplete type error when trying to invoke llvm::is_trivially_copyable on
+// clang::Module that contains a member PointerIntPair
+template<typename PointerTy, unsigned IntBits, typename IntType, typename PtrTraits, typename Info>
----------------
This comment doesn't really make sense here...

I would say something more about the *effect* of this, not the symptom that made you add it. So, talk about the fact that this allows even an incomplete `PointerIntPair` type (or some such) to be known to be trivially copyable.


================
Comment at: include/llvm/Support/type_traits.h:123-124
 
+// An implementation of `std::is_trivially_copyable` since STL version is not equally supported by all compilers, especially GCC 4.9.
+// Uniform implementation of this trait is important for ABI compatibility as it has an impact on SmallVector's ABI
+template <typename T>
----------------
Can you re-flow this comment to fit in 80-columns?


================
Comment at: include/llvm/Support/type_traits.h:128-154
+  // copy constructors
+  static constexpr bool has_trivial_copy_constructor =
+      std::is_copy_constructible<detail::trivial_helper<T>>::value;
+  static constexpr bool has_deleted_copy_constructor =
+      !std::is_copy_constructible<T>::value;
+
+  // move constructors
----------------
All of this should be private, no?


================
Comment at: include/llvm/Support/type_traits.h:169-175
+template <typename T>
+struct is_trivially_copyable<T*> {
+  static constexpr bool value = true;
+#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");
+#endif
+};
----------------
Why is this needed? Due to incomplete types? (answer in a comment please!)


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

https://reviews.llvm.org/D54472