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

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

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.

Taking a look at the gcc 5.4 failure now.

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

Properly update the patch to incorporate the separate static_asserts.

Split conditions into individual static_asserts.

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

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

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.

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

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.

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.

In D92514#2461810, @dblaikie wrote:

In D92514#2461808, @jplayer-nv wrote:

I spent some time root causing an llvm::Optional build error on MSVC 16.8.3 today related to using std::is_trivially_copyable to set llvm::is_trivially_copyable<T>::value. It looks like the same failure mentioned in this review.

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, 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 might need to explicitly check is_trivially_copy_constructible too since it might be possible the copy constructor is deleted.

All sounds pretty plausible to me - if you want to send a patch to add those checks?

I spent some time root causing an llvm::Optional build error on MSVC 16.8.3 today related to using std::is_trivially_copyable to set llvm::is_trivially_copyable<T>::value. It looks like the same failure mentioned in this review.