[ADT] Fix Optional<> with llvm::is_trivially_move_constructible

[steven_wu]

Summary

Fix the compatibility of Optional<> with some GCC versions that it will fail
to compile when T is getting checked for is_trivially_move_constructible
as mentioned here: https://reviews.llvm.org/D93510#2538983

Fix the problem by using llvm::is_trivially_move_constructible.

[steven_wu]

Rename the variables to be more accurate.

[dexonsmith]

This LGTM, but it'd be great if someone involved in the discussion for https://reviews.llvm.org/D93510 could chime in to confirm this isn't too surprising.

llvm/include/llvm/ADT/Optional.h

36–42

I think this comment should be expanded to cover is_trivially_move_constructible as well.

[dblaikie]

Probably should add some text to the comment explaining why this workaround is needed.

[steven_wu]

Address review feedback.

[jplayer-nv]

llvm/include/llvm/ADT/Optional.h

52–53

I don't have a test in mind yet, but any concern for the same instantiation bug with std::is_trivially_move_assignable pre-gcc7.4?

[steven_wu]

Fix test name.

[steven_wu]

llvm/include/llvm/ADT/Optional.h

52–53

I am not sure and we also do not have a LLVM version of that implementation. Maybe we can address that in a separate patch if that is a problem?

[jplayer-nv]

we also do not have a LLVM version of that implementation.

Good point.

Change looks good to me. Thanks!

[tatyana-krasnukha]

LGTM, thank you for this fix!

[steven_wu]

Rebase to TOT

[steven_wu]

clang-format

llvm/include/llvm/ADT/Optional.h

 namespace llvm {
 
 class raw_ostream;
 
 namespace optional_detail {
 
 /// Storage for any type.
 //
 // The specialization condition intentionally uses
-// llvm::is_trivially_copy_constructible instead of
-// std::is_trivially_copy_constructible.  GCC versions prior to 7.4 may
-// instantiate the copy constructor of `T` when
-// std::is_trivially_copy_constructible is instantiated.  This causes
-// compilation to fail if we query the trivially copy constructible property of
-// a class which is not copy constructible.
+// llvm::is_trivially_{copy/move}_constructible instead of
+// std::is_trivially_{copy/move}_constructible. GCC versions prior to 7.4 may
+// instantiate the copy/move constructor of `T` when
+// std::is_trivially_{copy/move}_constructible is instantiated.  This causes
+// compilation to fail if we query the trivially copy/move constructible
+// property of a class which is not copy/move constructible.
 //
 // The current implementation of OptionalStorage insists that in order to use
 // the trivial specialization, the value_type must be trivially copy
 // constructible and trivially copy assignable due to =default implementations
 // of the copy/move constructor/assignment.  It does not follow that this is
 // necessarily the case std::is_trivially_copyable is true (hence the expanded
 // specialization condition).
 //
 // The move constructible / assignable conditions emulate the remaining behavior
 // of std::is_trivially_copyable.
-template <typename T, bool = (llvm::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))>
+template <typename T,
+          bool = (llvm::is_trivially_copy_constructible<T>::value &&
+                  (llvm::is_trivially_move_constructible<T>::value ||
 class OptionalStorage {
   union {
     char empty;
     T value;
   };
   bool hasVal;
 
 public:

llvm/unittests/ADT/OptionalTest.cpp

TEST(OptionalTest, HashValue) {
   EXPECT_NE(hash_value(I0), hash_value(I));
   EXPECT_NE(hash_value(I1), hash_value(I));
   EXPECT_NE(hash_value(I1), hash_value(I0));
 
   // Check None hash the same way regardless of type.
   EXPECT_EQ(hash_value(B), hash_value(I));
 }
 
+struct NotTriviallyCopyable {
+  NotTriviallyCopyable(); // Constructor out-of-line.
+  virtual ~NotTriviallyCopyable() = default;
+  Optional<MoveOnly> MO;
+};
+
+TEST(OptionalTest, GCCIsTriviallyMoveConstructibleCompat) {
+  Optional<NotTriviallyCopyable> V;
+  EXPECT_FALSE(V);
+}
+
 } // end anonymous namespace