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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D93510#2499262 <https://reviews.llvm.org/D93510#2499262>, @jplayer-nv wrote:

> I just triaged the gcc 5.4 build break.  It appears that `std::is_trivially_copy_constructible<T>` instantiates the copy constructor (which is... strange).  So we can't query a compile-time property without the compiler forcing instantiation of the deleted copy constructor in this case.
>
> This bug exists all the way up through gcc 7.3.  Here's a godbolt link which reproduces the problem:
> https://godbolt.org/z/ETW7f1
>
> If we use `llvm::is_trivially_copy_constructible<T>` instead of `std::is_trivially_copy_constructible<T>`, then everything appears to be fine.
>
> Interestingly, `std::is_trivially_copy_assignable<T>` appears not to instantiate the copy assignment because the compiler isn't complaining about its use.  So I'm changing the `OptionalStorage` specialization condition to:
>
>   template <typename T, bool = (is_trivially_copy_constructible<T>::value &&
>                                 std::is_trivially_copy_assignable<T>::value &&
>                                 (std::is_trivially_move_constructible<T>::value ||
>                                  !std::is_move_constructible<T>::value) &&
>                                 (std::is_trivially_move_assignable<T>::value ||
>                                  !std::is_move_assignable<T>::value))>

Probably worth a big comment explaining all the quirks here in the code for future readers & I'd probably suggest qualifying the unqualified one as llvm:: even though that's redundant, to make it clearer that it's intentional and not just a missed 'std::'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93510