[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-38
+template <typename T, bool = is_trivially_copyable<T>::value
+                          &&std::is_trivially_copy_constructible<T>::value
+                              &&std::is_trivially_copy_assignable<T>::value>
----------------
jplayer-nv wrote:
> dblaikie wrote:
> > Could you run this through clang-format?
> This was already run through clang-format.
Ah, clang-format is getting confused about whether these are types or values. If you put some () around the whole expression (`bool = (x && y && z)`) it tidies this up a lot.


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


================
Comment at: llvm/unittests/ADT/OptionalTest.cpp:411-412
+  int Val{0};
+};
+
+TEST(OptionalTest, DeletedCopyConstructor) {
----------------
jplayer-nv wrote:
> 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.
I'd prefer to add it, and remove it with a comment if it does break something, so we know what we're working around and why.

Yeah, we aren't testing the host compiler - but it's pretty non-trivial how this type produces the specific features that are interesting to the test, so validating that those features have been created as intended seems valuable to me. Especially if there's a good chance a compiler might not be implementing them correctly & thus could lead to confusing test behavior, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93510