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

Tue Jan 5 13:09:19 PST 2021

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
----------------
jplayer-nv wrote:
> dblaikie wrote:
> > jplayer-nv wrote:
> > > dblaikie wrote:
> > > > jplayer-nv wrote:
> > > > > dblaikie wrote:
> > > > > > 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.
> > > > > You don't need a class to be trivially movable or trivially move assignable in order to use the trivial `OptionalStorage` specialization.  The move constructor / assignment can be deleted, and the compiler will fall back to the copy constructor / assignment.
> > > > > 
> > > > > We just need to verify that the user hasn't specified a custom move constructor / assignment.  This is perfectly captured by `is_trivially_copyable`.  That is, if either the move constructor or assignment are deleted, they will not be "eligible" in the spec, and therefore will not disqualify the class from being trivially copyable.
> > > > > 
> > > > > If we were to expand this out, I think it would look like:
> > > > > ```
> > > > >    std::is_trivially_copy_constructible<T>::value
> > > > > && std::is_trivially_copy_assignable<T>::value
> > > > > && (!std::is_move_constructible<T>::value || std::is_trivially_move_constructible<T>::value)
> > > > > && (!std::is_move_assignable<T>::value || std::is_trivially_move_assignable<T>::value)
> > > > > ```
> > > > That doesn't sound quite right to me. If something isn't move constructible or isn't move assignable, wouldn't that break the places where Optional tries to move them?
> > > > 
> > > > > The move constructor / assignment can be deleted, and the compiler will fall back to the copy constructor / assignment.
> > > > 
> > > > I don't think that's quite right - if something has an explicitly deleted move constructor, it'll be "!std::is_move_constructible" and move construction expressions will fail to compile, like this: https://godbolt.org/z/36xfb7
> > > > 
> > > > So I think your proposed expansion is equivalent to:
> > > > ```
> > > >    std::is_trivially_copy_constructible<T>::value
> > > > && std::is_trivially_copy_assignable<T>::value
> > > > && std::is_trivially_move_constructible<T>::value
> > > > && std::is_trivially_move_assignable<T>::value
> > > > ```
> > > > 
> > > > Which seems like it might be nice to have it written out that way?
> > > > The move constructor / assignment can be deleted, and the compiler will fall back to the copy constructor / assignment.
> > > > 
> > > >> I don't think that's quite right - if something has an explicitly deleted move constructor, it'll be "!std::is_move_constructible" and move construction expressions will fail to compile, like this: https://godbolt.org/z/36xfb7
> > > 
> > > I must be missing something, because the code example you provided seems to reinforce my intended point.  I'll admit that I overstated, but it applies when dealing with a field of another class.  Here's a trimmed version of the code example from your last comment:
> > > https://godbolt.org/z/naE4PY
> > > 
> > > Even though `t1` has its move constructor explicitly deleted, `t2` is still trivially movable.  No compile error when we try to move construct the class which contains a field with a deleted move constructor.
> > > 
> > > I did add a test to demonstrate this property of `Optional`.  If its `value_type` is not movable, then compilation will not fail when the `Optional` is moved.
> > > Even though t1 has its move constructor explicitly deleted, t2 is still trivially movable. 
> > 
> > Yep, I'm with you there - with or without the explicit defaults (in your t2 or my t2), the type is trivially movable.
> > 
> > > No compile error when we try to move construct the class which contains a field with a deleted move constructor.
> > 
> > Yep, agreed.
> > 
> > > I did add a test to demonstrate this property of Optional. If its value_type is not movable,
> > 
> > What do you mean by "not movable" here? Note that t2 in both our examples returns true for "is_move_constructible" despite the fact that move construction will invoke a copy constructor instead.
> > 
> > I think the specific thing I'm looking for (could you provide this in the form of a struct definition, preferrably in a godbolt link) is a case where this function returns false:
> > ```
> > #include <type_traits>
> > template <typename T>
> > bool test() {
> >   return (std::is_trivially_copy_constructible<T>::value &&
> >           std::is_trivially_copy_assignable<T>::value &&
> >           (!std::is_move_constructible<T>::value ||
> >            std::is_trivially_move_constructible<T>::value) &&
> >           (!std::is_move_assignable<T>::value ||
> >            std::is_trivially_move_assignable<T>::value)) ==
> >          (std::is_trivially_copy_constructible<T>::value &&
> >           std::is_trivially_copy_assignable<T>::value &&
> >           std::is_trivially_move_constructible<T>::value &&
> >           std::is_trivially_move_assignable<T>::value);
> > }
> > ```
> > 
> > If there is no such type for which that function returns false, then I think the simpler of these two expression (is_trivially_{copy,move}_{assignable,constructible}) would be the preferable/tidier/easier to read expression of the intent here.
> >> I did add a test to demonstrate this property of Optional. If its value_type is not movable,
> > 
> > What do you mean by "not movable" here? Note that t2 in both our examples returns true for "is_move_constructible" despite the fact that move construction will invoke a copy constructor instead.
> 
> I mean that `std::is_move_constructible<Optional<Foo>::value_type>::value == false`.  In this case, `Optional` will invoke the copy constructor as long as `Foo` is trivially copy constructible.
> 
> 
> > I think the specific thing I'm looking for (could you provide this in the form of a struct definition, preferrably in a godbolt link) is a case where this function returns false:
> 
> Sure thing.
> https://godbolt.org/z/ccxj6n
> 
> The simpler condition is unnecessarily conservative when the move constructor / assignment are deleted.
> I mean that std::is_move_constructible<Optional<Foo>::value_type>::value == false. In this case, Optional will invoke the copy constructor as long as Foo is trivially copy constructible.

OK, good to understand - thanks for helping me through it. Sorry this is all a bit of a slog.

> Sure thing.
> https://godbolt.org/z/ccxj6n

Ah, thanks!

> The simpler condition is unnecessarily conservative when the move constructor / assignment are deleted.

Fair enough - not sure it's necessarily an important entity to optimize for (trivially copyable, but deleted move op is a bit weird), but glad to understand/OK with the direction you're proposing here.

================
Comment at: llvm/unittests/ADT/OptionalTest.cpp:411-412
+  int Val{0};
+};
+
+TEST(OptionalTest, DeletedCopyConstructor) {
----------------
jplayer-nv wrote:
> dblaikie wrote:
> > 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.
> Looks like the static asserts are firing.  So will comment them out when we converge on the `OptionalStorage` specialization condition.
Firing in some pre-merge checks? Got some links/details on the failures?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93510