[PATCH] D92514: Switch from llvm::is_trivially_copyable to std::is_trivially_copyable

[James Player via Phabricator via llvm-commits]

jplayer-nv added a comment.

In D92514#2461810 <https://reviews.llvm.org/D92514#2461810>, @dblaikie wrote:

> In D92514#2461808 <https://reviews.llvm.org/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 <https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable>, 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?

Sure... just getting my feet wet in LLVM, so may take some time to generate a patch / figure out how to submit it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92514