[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:
> > > > 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.
> Even though t1 has its move constructor explicitly deleted, t2 is still trivially movable. 

Yep, I'm with you there - with or without the explicit defaults (in your t2 or my t2), the type is trivially movable.

> No compile error when we try to move construct the class which contains a field with a deleted move constructor.

Yep, agreed.

> I did add a test to demonstrate this property of Optional. If its value_type is not movable,

What do you mean by "not movable" here? Note that t2 in both our examples returns true for "is_move_constructible" despite the fact that move construction will invoke a copy constructor instead.

I think the specific thing I'm looking for (could you provide this in the form of a struct definition, preferrably in a godbolt link) is a case where this function returns false:
```
#include <type_traits>
template <typename T>
bool test() {
  return (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)) ==
         (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);
}
```

If there is no such type for which that function returns false, then I think the simpler of these two expression (is_trivially_{copy,move}_{assignable,constructible}) would be the preferable/tidier/easier to read expression of the intent here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93510