[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
----------------
Is the "is_trivially_copyable" useful/necessary anymore? Would it be better to remove it (perhaps we need "is_trivially_destructible" in its place)?


================
Comment at: llvm/unittests/ADT/OptionalTest.cpp:398
+
+  // Delete the volatile copy constructor.
+  NonTCopy(volatile NonTCopy const &) = delete;
----------------
This comment might be too literal (it doesn't seem to add a lot compared to reading the line of source code) - maybe an explanation of why deleting the volatile copy constructor is important/relevant would be helpful?

(similar in the other class)


================
Comment at: llvm/unittests/ADT/OptionalTest.cpp:411-412
+  int Val{0};
+};
+
+TEST(OptionalTest, DeletedCopyConstructor) {
----------------
Could you throw some static assertions in here to match the comment for this class and show that it's trivially copyable but not trivially copy constructible?

(similar for the other class)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93510