[PATCH] D54472: Disable invalid isPodLike<> specialization

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

While I like the update (keeping any potential changes to `Optional` and `SmallVector` separate seems good) I have a serious concern that this has the same fundamental problem as the prior mentioned ABI issue.

Specifically, if we end up mixing .o files from GCC and Clang, they may have subtly different ABIs due to different types being considered "trivially copyable". That can in turn cause these .o files to expect different specializations of `SmallVector` (or any other type utilizing this trait). And that in turn can lead to horrible ABI issues.

So, at the very least, I do not think we can mix `std::is_trivial` and `std::is_trivially_copyable` in this way.

If we want to do this, we have to use predicates that will be *ABI stable* between compilers on the same platform where we might reasonably mix .o files. This is... challenging.

I believe the most portable and reliably implementations I'm aware of don't *technically* implement `std::is_trivially_copyable`. Instead, they separately implement `std::is_trivially_copy_constructible` and `std::is_trivially_copy_assignable`. I believe we could change LLVM's existing usag of `isPodLike` to use these traits and then implement them using the same core technique used by Abseil here:
https://github.com/abseil/abseil-cpp/blob/master/absl/meta/type_traits.h#L291-L295
https://github.com/abseil/abseil-cpp/blob/master/absl/meta/type_traits.h#L325-L329

I'm still not 100% sure this is going to be reliable from an ABI perspective. I'm asking a group of Abseil and C++ ABI experts to see if they see any problems here.



================
Comment at: include/llvm/ADT/SmallVector.h:185
+/// put method implementations that are designed to work with non-POD-like T's.
+template <typename T, bool is_trivially_copyable>
 class SmallVectorTemplateBase : public SmallVectorTemplateCommon<T> {
----------------
Quuxplusone wrote:
> Incidentally, unless there's a portability issue I'm not aware of, I would prefer to write this as
> 
>     template <typename T, bool = is_trivially_copyable<T>::value>
>     class SmallVectorTemplateBase : public SmallVectorTemplateCommon<T> {
> 
> so that on line ~322 you could write simply
> 
>     template <typename T>
>     class SmallVectorImpl : public SmallVectorTemplateBase<T> {
>       using SuperClass = SmallVectorTemplateBase<T>;
> 
> without repeating your `is_trivially_copyable<T>::value` all over the place.
> 
> The more times you have to manually type out `is_trivially_copyable<T>::value`, the greater the chance you'll slip up and write `std::is_trivially_copyable<T>::value` on autopilot.
While this might be an independently nice improvement, I agree w/ the current mechanics of the patch to make the diff more isolated.


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

https://reviews.llvm.org/D54472