Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable

[jplayer-nv]

Summary

Current code breaks this version of MSVC due to a mismatch between std::is_trivially_copyable and llvm::is_trivially_copyable for std::pair instantiations. Hence I was attempting to use std::is_trivially_copyable to set llvm::is_trivially_copyable<T>::value.

I spent some time root causing an llvm::Optional build error on MSVC 16.8.3 related to the change described above:

62>C:\src\ocg_llvm\llvm-project\llvm\include\llvm/ADT/BreadthFirstIterator.h(96,12): error C2280: 'llvm::Optional<std::pair<std::pair<unsigned int,llvm::Graph<4>::NodeSubset> *,llvm::Optional<llvm::Graph<4>::ChildIterator>>> &llvm::Optional<std::pair<std::pair<unsigned int,llvm::Graph<4>::NodeSubset> *,llvm::Optional<llvm::Graph<4>::ChildIterator>>>::operator =(const llvm::Optional<std::pair<std::pair<unsigned int,llvm::Graph<4>::NodeSubset> *,llvm::Optional<llvm::Graph<4>::ChildIterator>>> &)': attempting to reference a deleted function (compiling source file C:\src\ocg_llvm\llvm-project\llvm\unittests\ADT\BreadthFirstIteratorTest.cpp)
...

The "trivial" specialization of optional_detail::OptionalStorage assumes that the value type is trivially copy constructible and trivially copy assignable. The specialization is invoked based on a check of is_trivially_copyable alone, which does not imply both is_trivially_copy_assignable and is_trivially_copy_constructible are true.

According to the spec, a deleted assignment operator does not make is_trivially_copyable false. So I think all these properties need to be checked explicitly in order to specialize OptionalStorage to the "trivial" version:

/// Storage for any type.
template <typename T, bool = std::is_trivially_copy_constructible<T>::value
                          && std::is_trivially_copy_assignable<T>::value>
class OptionalStorage {

Above fixed my build break in MSVC, but I think we need to explicitly check is_trivially_copy_constructible too since it might be possible the copy constructor is deleted. Also would be ideal to move over to std::is_trivially_copyable instead of the llvm namespace verson.

[dblaikie]

any idea why this only fails for MSVC? It'd be good to understand more why this didn't trip up other builds so we can avoid those sort of divergences in the future, perhaps.

Would it be possible to add a test case for the is_trivially_copy_constructible case you speculated about?

llvm/include/llvm/ADT/Optional.h

36–60

Could you run this through clang-format?

[jplayer-nv]

1. Use std::is_trivially_copyable instead of llvm::is_trivially_copyable in Optional storage template specialization logic.
2. Added test cases to OptionalTest.cpp which will fail compilation (verified on MSVC 16.8.3) if new conditions are absent from Optional storage template specialization logic.

[jplayer-nv]

any idea why this only fails for MSVC? It'd be good to understand more why this didn't trip up other builds so we can avoid those sort of divergences in the future, perhaps.

Likely related to a recent change to std::pair in MSVC. They added the following:

pair& operator=(const volatile pair&) = delete;

This seems to implicitly delete the default version of the copy assignment. Although, they do have a non-trivial template which serves as the copy assignment in place the the deleted default one. Not sure whether MS intended to delete the default copy assignment here, but this is valid C++ code as far as I can tell.

The test cases I added to OptionalTest.cpp should replicate these conditions for both the copy assignment and the copy constructor.

llvm/include/llvm/ADT/Optional.h

36–60

This was already run through clang-format.

[dblaikie]

llvm/include/llvm/ADT/Optional.h

36–59

Is the "is_trivially_copyable" useful/necessary anymore? Would it be better to remove it (perhaps we need "is_trivially_destructible" in its place)?

llvm/unittests/ADT/OptionalTest.cpp

399

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)

412–413

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)

[jplayer-nv]

Clarified comments above deleted volatile copy constructor / assignment in test classes.

llvm/include/llvm/ADT/Optional.h

36–59

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.

llvm/unittests/ADT/OptionalTest.cpp

412–413

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.

[dblaikie]

llvm/include/llvm/ADT/Optional.h

36–59

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.

36–60

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.

llvm/unittests/ADT/OptionalTest.cpp

412–413

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.

[jplayer-nv]

1. Expanded the check for the trivial specialization of OptionalStorage
2. Formatted the OptionalStorage template list.
3. Added a test to ensure that deleting move constructor / assignment will still choose the trivial OptionalStorage specialization.
4. Added requests static_asserts to validate class properties in Optional testing code.

[jplayer-nv]

llvm/include/llvm/ADT/Optional.h

36–59

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)

[dblaikie]

llvm/include/llvm/ADT/Optional.h

36–59

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?

[jplayer-nv]

llvm/include/llvm/ADT/Optional.h

36–59

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.

[dblaikie]

llvm/include/llvm/ADT/Optional.h

36–59

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.

[jplayer-nv]

llvm/include/llvm/ADT/Optional.h

36–59

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.

llvm/unittests/ADT/OptionalTest.cpp

412–413

Looks like the static asserts are firing. So will comment them out when we converge on the OptionalStorage specialization condition.

[dblaikie]

llvm/include/llvm/ADT/Optional.h

36–59

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.

llvm/unittests/ADT/OptionalTest.cpp

412–413

Firing in some pre-merge checks? Got some links/details on the failures?

[jplayer-nv]

llvm/include/llvm/ADT/Optional.h

36–59

No worries. This is all very pedantic.

llvm/unittests/ADT/OptionalTest.cpp

412–413

Correct. Windows passed, but debian and clang-tidy both failed on the static_asserts:

Windows
Debian
clang-tidy

[jplayer-nv]

Added preprocessing around static_asserts added to OptionalTests.cpp to only include them in recent windows builds.

[jplayer-nv]

With current change, pre-merge checks pass. I think I've resolved all issues / requests.

[dblaikie]

Looks good - thanks!

llvm/unittests/ADT/OptionalTest.cpp

417–421

Probably best to split that into two static_asserts, so it's easier to diagnose failures.

[jplayer-nv]

Split conditions into individual static_asserts.

[jplayer-nv]

Properly update the patch to incorporate the separate static_asserts.

[jplayer-nv]

@dblaikie, I don't have commit access. Are you the right person to ask to commit this patch on my behalf?

[aganea]

llvm/unittests/ADT/OptionalTest.cpp

416

When compiling with clang-cl 11.0 into a "x64 Native Tools Command Prompt for VS 2019" (16.8.4) cmd.exe, I see:

F:\aganea\llvm-project\llvm\unittests\ADT\OptionalTest.cpp(417,1): error: static_assert failed due to requirement
      'std::is_trivially_copyable<(anonymous namespace)::NonTCopy>::value' "Expect NonTCopy to be trivially copyable"
static_assert(std::is_trivially_copyable<NonTCopy>::value,
^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
F:\aganea\llvm-project\llvm\unittests\ADT\OptionalTest.cpp(465,1): error: static_assert failed due to requirement
      'std::is_trivially_copyable<(anonymous namespace)::NonTAssign>::value' "Expect NonTAssign to be trivially copyable"
static_assert(std::is_trivially_copyable<NonTAssign>::value,
^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.

This is because Clang's -fms-compatibility-version inherits its version from the cl.exe in %PATH%.

I've tried with Clang 11.0 and Clang 12.0 at HEAD (rGeec856848ccc481b2704ebf64d725e635a3d7dca). They both insist that NonTCopy isn't __is_trivially_copyable but MSVC 16.8.4 and GCC at HEAD says it is.

[dblaikie]

llvm/unittests/ADT/OptionalTest.cpp

416

Since I don't have a setup for this - would you mind testing (& if it works, committing) adding a && !defined(__clang__) to these #ifs?

[RKSimon]

@jplayer-nv It looks like this patch has broken the ppc buildbots - please can you take a look? http://lab.llvm.org:8011/#/builders/93/builds/1435

[ArcsinX]

It seems that this patch breaks clangd build with GCC 5.4 (reverting this patch fixes the build)

[2011/2198] Building CXX object tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/CodeComplete.cpp.o
FAILED: /usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/tools/extra/clangd -I/mnt/d/work/llvm-project/clang-tools-extra/clangd -Itools/clang/tools/extra/clangd/../clang-tidy -I/mnt/d/work/llvm-project/clang/include -Itools/clang/include -Iinclude -I/mnt/d/work/llvm-project/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3 -DNDEBUG  -fno-exceptions -fno-rtti -std=c++14 -MD -MT tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/CodeComplete.cpp.o -MF tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/CodeComplete.cpp.o.d -o tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/CodeComplete.cpp.o -c /mnt/d/work/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp
In file included from /mnt/d/work/llvm-project/llvm/include/llvm/ADT/StringMap.h:16:0,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/CompileCommands.h:14,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/GlobalCompilationDatabase.h:12,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/Compiler.h:18,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/CodeComplete.h:18,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:20:
/mnt/d/work/llvm-project/llvm/include/llvm/ADT/StringMapEntry.h: In instantiation of ‘llvm::StringMapEntryStorage<ValueTy>::StringMapEntryStorage(size_t, InitTy&& ...) [with InitTy = {std::unique_ptr<clang::clangd::FileDistance, std::default_delete<clang::clangd::FileDistance> >&}; ValueTy = std::unique_ptr<clang::clangd::FileDistance>; size_t = long unsigned int]’:
/mnt/d/work/llvm-project/llvm/include/llvm/ADT/StringMapEntry.h:71:41:   required from ‘static llvm::StringMapEntry<ValueTy>* llvm::StringMapEntry<ValueTy>::Create(llvm::StringRef, AllocatorTy&, InitTy&& ...) [with AllocatorTy = llvm::MallocAllocator; InitTy = {std::unique_ptr<clang::clangd::FileDistance, std::default_delete<clang::clangd::FileDistance> >&}; ValueTy = std::unique_ptr<clang::clangd::FileDistance>]’
/mnt/d/work/llvm-project/llvm/include/llvm/ADT/StringMap.h:158:39:   required from ‘llvm::StringMap<ValueTy, AllocatorTy>::StringMap(const llvm::StringMap<ValueTy, AllocatorTy>&) [with ValueTy = std::unique_ptr<clang::clangd::FileDistance>; AllocatorTy = llvm::MallocAllocator]’
/mnt/d/work/llvm-project/clang-tools-extra/clangd/FileDistance.h:93:7:   required from ‘struct std::is_trivially_copy_constructible<clang::clangd::URIDistance>’
/mnt/d/work/llvm-project/llvm/include/llvm/ADT/Optional.h:36:78:   required from ‘class llvm::Optional<clang::clangd::URIDistance>’
/mnt/d/work/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:1272:31:   required from here
/mnt/d/work/llvm-project/llvm/include/llvm/ADT/StringMapEntry.h:47:49: error: use of deleted function ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = clang::clangd::FileDistance; _Dp = std::default_delete<clang::clangd::FileDistance>]’
         second(std::forward<InitTy>(initVals)...) {}
                                                 ^
In file included from /usr/include/c++/5/memory:81:0,
                 from /mnt/d/work/llvm-project/llvm/include/llvm/ADT/Optional.h:23,
                 from /mnt/d/work/llvm-project/llvm/include/llvm/ADT/STLExtras.h:19,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/support/Context.h:17,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/support/Threading.h:12,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/CompileCommands.h:11,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/GlobalCompilationDatabase.h:12,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/Compiler.h:18,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/CodeComplete.h:18,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:20:
/usr/include/c++/5/bits/unique_ptr.h:356:7: note: declared here
       unique_ptr(const unique_ptr&) = delete;

[aganea]

llvm/unittests/ADT/OptionalTest.cpp

416

I've reverted the patch since I think it needs a bit more testing.

@dblaikie @rsmith Do you think the NonTCopy test case above requires fixes in Clang? (so that __is_trivially_copyable returns true like the other compilers)

[jplayer-nv]

Taking a look at the gcc 5.4 failure now.

Now that the patch has been committed & reverted, do I start a new patch when fixes for identified issues are incorporated?

[dblaikie]

In D93510#2498718, @jplayer-nv wrote:

Taking a look at the gcc 5.4 failure now.

Now that the patch has been committed & reverted, do I start a new patch when fixes for identified issues are incorporated?

Fine to reopen and reuse this one.

[jplayer-nv]

I just triaged the gcc 5.4 build break. It appears that std::is_trivially_copy_constructible<T> instantiates the copy constructor (which is... strange). So we can't query a compile-time property without the compiler forcing instantiation of the deleted copy constructor in this case.

This bug exists all the way up through gcc 7.3. Here's a godbolt link which reproduces the problem:
https://godbolt.org/z/ETW7f1

If we use llvm::is_trivially_copy_constructible<T> instead of std::is_trivially_copy_constructible<T>, then everything appears to be fine.

Interestingly, std::is_trivially_copy_assignable<T> appears not to instantiate the copy assignment because the compiler isn't complaining about its use. So I'm changing the OptionalStorage specialization condition to:

template <typename T, bool = (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))>

[dblaikie]

In D93510#2499262, @jplayer-nv wrote:

I just triaged the gcc 5.4 build break. It appears that std::is_trivially_copy_constructible<T> instantiates the copy constructor (which is... strange). So we can't query a compile-time property without the compiler forcing instantiation of the deleted copy constructor in this case.

This bug exists all the way up through gcc 7.3. Here's a godbolt link which reproduces the problem:
https://godbolt.org/z/ETW7f1

If we use llvm::is_trivially_copy_constructible<T> instead of std::is_trivially_copy_constructible<T>, then everything appears to be fine.

Interestingly, std::is_trivially_copy_assignable<T> appears not to instantiate the copy assignment because the compiler isn't complaining about its use. So I'm changing the OptionalStorage specialization condition to:

template <typename T, bool = (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))>

Probably worth a big comment explaining all the quirks here in the code for future readers & I'd probably suggest qualifying the unqualified one as llvm:: even though that's redundant, to make it clearer that it's intentional and not just a missed 'std::'

[aganea]

llvm/unittests/ADT/OptionalTest.cpp

416

@jplayer-nv Could you please also add && !defined(__clang__) like suggested by David above? Same thing below at L464.

[jplayer-nv]

Added !defined(__clang__) conditions to static_assert preprocessing. Also added large comment describing the mental gymnastics of the OptionalStorage specialization condition.

[dblaikie]

Looks worth another go

[jplayer-nv]

Quick reminder that I don't have commit access, and need someone to commit this patch on my behalf. Thanks!

[aganea]

I can do that. Let me test the patch on my end and I'll commit.

[tatyana-krasnukha]

In D93510#2499262, @jplayer-nv wrote:

I just triaged the gcc 5.4 build break. It appears that std::is_trivially_copy_constructible<T> instantiates the copy constructor (which is... strange). So we can't query a compile-time property without the compiler forcing instantiation of the deleted copy constructor in this case.

This bug exists all the way up through gcc 7.3. Here's a godbolt link which reproduces the problem:
https://godbolt.org/z/ETW7f1

GCC 7.3 produces the same error for std::is_trivially_move_constructible<T> if T is not trivially constructible because of having a virtual destructor. Just add virtual ~URIDistance() = default; to your godbolt example to reproduce.
This issue breaks LLDB unit tests build (Utility/ReproducerInstrumentationTest.cpp).

llvm/include/llvm/ADT/Optional.h

 
 class raw_ostream;
 
 namespace optional_detail {
 
 struct in_place_t {};
 
 /// Storage for any type.
-template <typename T, bool = is_trivially_copyable<T>::value>
+//
+// 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.
+//
+// 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))>
 class OptionalStorage {
   union {
     char empty;
     T value;
   };
   bool hasVal;
 
 public:
   ~OptionalStorage() { reset(); }

llvm/unittests/ADT/OptionalTest.cpp

 //===- llvm/unittest/ADT/OptionalTest.cpp - Optional unit tests -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest-spi.h"
 #include "gtest/gtest.h"
 
 #include <array>
 
 
 using namespace llvm;
TEST(OptionalTest, ImmovableEmplace) {
   Immovable::ResetCounts();
   A.emplace(4);
   EXPECT_TRUE((bool)A);
   EXPECT_EQ(4, A->val);
   EXPECT_EQ(1u, Immovable::Constructions);
   EXPECT_EQ(0u, Immovable::Destructions);
 }
 
+// Craft a class which is_trivially_copyable, but not
+// is_trivially_copy_constructible.
+struct NonTCopy {
+  NonTCopy() = default;
+
+  // Delete the volatile copy constructor to engage the "rule of 3" and delete
+  // any unspecified copy assignment or constructor.
+  NonTCopy(volatile NonTCopy const &) = delete;
+
+  // Leave the non-volatile default copy constructor unspecified (deleted by
+  // rule of 3)
+
+  // This template can serve as the copy constructor, but isn't chosen
+  // by =default in a class with a 'NonTCopy' member.
+  template <typename Self = NonTCopy>
+  NonTCopy(Self const &Other) : Val(Other.Val) {}
+
+  NonTCopy &operator=(NonTCopy const &) = default;
+
+  int Val{0};
+};
+
+#if defined(_MSC_VER) && _MSC_VER >= 1927 && !defined(__clang__)
+// Currently only true on recent MSVC releases.
+static_assert(std::is_trivially_copyable<NonTCopy>::value,
+              "Expect NonTCopy to be trivially copyable");
+
+static_assert(!std::is_trivially_copy_constructible<NonTCopy>::value,
+              "Expect NonTCopy not to be trivially copy constructible.");
+#endif // defined(_MSC_VER) && _MSC_VER >= 1927
+
+TEST(OptionalTest, DeletedCopyConstructor) {
+
+  // Expect compile to fail if 'trivial' version of
+  // optional_detail::OptionalStorage is chosen.
+  using NonTCopyOptT = Optional<NonTCopy>;
+  NonTCopyOptT NonTCopy1;
+
+  // Check that the Optional can be copy constructed.
+  NonTCopyOptT NonTCopy2{NonTCopy1};
+
+  // Check that the Optional can be copy assigned.
+  NonTCopy1 = NonTCopy2;
+}
+
+// Craft a class which is_trivially_copyable, but not
+// is_trivially_copy_assignable.
+class NonTAssign {
+public:
+  NonTAssign() = default;
+  NonTAssign(NonTAssign const &) = default;
+
+  // Delete the volatile copy assignment to engage the "rule of 3" and delete
+  // any unspecified copy assignment or constructor.
+  NonTAssign &operator=(volatile NonTAssign const &) = delete;
+
+  // Leave the non-volatile default copy assignment unspecified (deleted by rule
+  // of 3).
+
+  // This template can serve as the copy assignment, but isn't chosen
+  // by =default in a class with a 'NonTAssign' member.
+  template <typename Self = NonTAssign>
+  NonTAssign &operator=(Self const &Other) {
+    A = Other.A;
+    return *this;
+  }
+
+  int A{0};
+};
+
+#if defined(_MSC_VER) && _MSC_VER >= 1927 && !defined(__clang__)
+// Currently only true on recent MSVC releases.
+static_assert(std::is_trivially_copyable<NonTAssign>::value,
+              "Expect NonTAssign to be trivially copyable");
+
+static_assert(!std::is_trivially_copy_assignable<NonTAssign>::value,
+              "Expect NonTAssign not to be trivially assignable.");
+#endif // defined(_MSC_VER) && _MSC_VER >= 1927
+
+TEST(OptionalTest, DeletedCopyAssignment) {
+
+  // Expect compile to fail if 'trivial' version of
+  // optional_detail::OptionalStorage is chosen.
+  using NonTAssignOptT = Optional<NonTAssign>;
+  NonTAssignOptT NonTAssign1;
+
+  // Check that the Optional can be copy constructed.
+  NonTAssignOptT NonTAssign2{NonTAssign1};
+
+  // Check that the Optional can be copy assigned.
+  NonTAssign1 = NonTAssign2;
+}
+
+struct NoTMove {
+  NoTMove() = default;
+  NoTMove(NoTMove const &) = default;
+  NoTMove &operator=(NoTMove const &) = default;
+
+  // Delete move constructor / assignment.  Compiler should fall-back to the
+  // trivial copy constructor / assignment in the trivial OptionalStorage
+  // specialization.
+  NoTMove(NoTMove &&) = delete;
+  NoTMove &operator=(NoTMove &&) = delete;
+
+  int Val{0};
+};
+
+TEST(OptionalTest, DeletedMoveConstructor) {
+  using NoTMoveOptT = Optional<NoTMove>;
+
+  NoTMoveOptT NonTMove1;
+  NoTMoveOptT NonTMove2{std::move(NonTMove1)};
+
+  NonTMove1 = std::move(NonTMove2);
+
+  static_assert(
+      std::is_trivially_copyable<NoTMoveOptT>::value,
+      "Expect Optional<NoTMove> to still use the trivial specialization "
+      "of OptionalStorage despite the deleted move constructor / assignment.");
+}
+
+class NoCopyStringMap {
+public:
+  NoCopyStringMap() = default;
+
+private:
+  llvm::StringMap<std::unique_ptr<int>> Map;
+};
+
+TEST(OptionalTest, DeletedCopyStringMap) {
+  // Old versions of gcc (7.3 and prior) instantiate the copy constructor when
+  // std::is_trivially_copyable is instantiated.  This test will fail
+  // compilation if std::is_trivially_copyable is used in the OptionalStorage
+  // specialization condition by gcc <= 7.3.
+  Optional<NoCopyStringMap> TestInstantiation;
+}
+
 #if LLVM_HAS_RVALUE_REFERENCE_THIS
 
 TEST(OptionalTest, MoveGetValueOr) {
   Optional<MoveOnly> A;
 
   MoveOnly::ResetCounts();
   EXPECT_EQ(42, std::move(A).getValueOr(MoveOnly(42)).val);
   EXPECT_EQ(1u, MoveOnly::MoveConstructions);