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

James Player via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 4 12:15:34 PST 2021


jplayer-nv marked 5 inline comments 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:
> > > 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)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93510



More information about the llvm-commits mailing list