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

[James Player via Phabricator via llvm-commits]

jplayer-nv marked an inline comment as done.
jplayer-nv 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
----------------
dblaikie wrote:
> 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?
> 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

I must be missing something, because the code example you provided seems to reinforce my intended point.  I'll admit that I overstated, but it applies when dealing with a field of another class.  Here's a trimmed version of the code example from your last comment:
https://godbolt.org/z/naE4PY

Even though `t1` has its move constructor explicitly deleted, `t2` is still trivially movable.  No compile error when we try to move construct the class which contains a field with a deleted move constructor.

I did add a test to demonstrate this property of `Optional`.  If its `value_type` is not movable, then compilation will not fail when the `Optional` is moved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93510