[PATCH] D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/Optional.h:36
 /// Storage for any type.
-template <typename T, bool = is_trivially_copyable<T>::value>
+template <typename T, bool = std::is_trivially_copyable<T>::value
+                          &&std::is_trivially_copy_constructible<T>::value
----------------
jplayer-nv wrote:
> dblaikie wrote:
> > jplayer-nv wrote:
> > > dblaikie wrote:
> > > > Is the "is_trivially_copyable" useful/necessary anymore? Would it be better to remove it (perhaps we need "is_trivially_destructible" in its place)?
> > > Yes I think it is.  It covers the `is_trivially_destructible` conditon, but it also covers non-trivial move assignment / constructor.  Pretty sure in the case of a deleted move constructor / assignment, we would fall back to the copy constructor / assignment (so current conditions should be sufficient to avoid build break).  At the same time, we want to honor any non-trivial move assignment / constructor that may exist.
> > Fair enough - so what would the fully expanded check look like? is_trivially_copy_constructible + is_trivially_copy_assignable + is_trivially_move_constructible + is_trivially_move_assignable + is_trivially_destructible? That might be more clear than as its written, even though it's more verbose.
> You don't need a class to be trivially movable or trivially move assignable in order to use the trivial `OptionalStorage` specialization.  The move constructor / assignment can be deleted, and the compiler will fall back to the copy constructor / assignment.
> 
> We just need to verify that the user hasn't specified a custom move constructor / assignment.  This is perfectly captured by `is_trivially_copyable`.  That is, if either the move constructor or assignment are deleted, they will not be "eligible" in the spec, and therefore will not disqualify the class from being trivially copyable.
> 
> If we were to expand this out, I think it would look like:
> ```
>    std::is_trivially_copy_constructible<T>::value
> && std::is_trivially_copy_assignable<T>::value
> && (!std::is_move_constructible<T>::value || std::is_trivially_move_constructible<T>::value)
> && (!std::is_move_assignable<T>::value || std::is_trivially_move_assignable<T>::value)
> ```
That doesn't sound quite right to me. If something isn't move constructible or isn't move assignable, wouldn't that break the places where Optional tries to move them?

> The move constructor / assignment can be deleted, and the compiler will fall back to the copy constructor / assignment.

I don't think that's quite right - if something has an explicitly deleted move constructor, it'll be "!std::is_move_constructible" and move construction expressions will fail to compile, like this: https://godbolt.org/z/36xfb7

So I think your proposed expansion is equivalent to:
```
   std::is_trivially_copy_constructible<T>::value
&& std::is_trivially_copy_assignable<T>::value
&& std::is_trivially_move_constructible<T>::value
&& std::is_trivially_move_assignable<T>::value
```

Which seems like it might be nice to have it written out that way?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93510