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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92514