[all-commits] [llvm/llvm-project] 854f09: Fix llvm::Optional build breaks in MSVC using std:...

[David Blaikie via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 854f0984f0b7ab9a9a541a4bcda7ea173e4113d3
      https://github.com/llvm/llvm-project/commit/854f0984f0b7ab9a9a541a4bcda7ea173e4113d3
  Author: James Player <jplayer at nvidia.com>
  Date:   2021-01-13 (Wed, 13 Jan 2021)

  Changed paths:
    M llvm/include/llvm/ADT/Optional.h
    M llvm/unittests/ADT/OptionalTest.cpp

  Log Message:
  -----------
  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.

[[ https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable | 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