HomePhabricator

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

Authored by jplayer-nv on Wed, Jan 13, 2:17 PM.

Description

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

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.

Reviewed By: dblaikie

Differential Revision: https://reviews.llvm.org/D93510