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

[James Player via Phabricator via llvm-commits]

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:
> 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.


================
Comment at: llvm/unittests/ADT/OptionalTest.cpp:411-412
+  int Val{0};
+};
+
+TEST(OptionalTest, DeletedCopyConstructor) {
----------------
dblaikie wrote:
> 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)
I thought about that but deleted them for fear of breaking a build.  With such a wide array of compilers building this code, one is bound to come up with an unexpected result.

I agree it would make the code easier to read, but I still don't think it's a good idea to incorporate into the commit.  My thought was that we're testing `llvm::Optional` here, not the host compiler.

If you feel strongly, I will add them back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93510