Page MenuHomePhabricator

jplayer-nv (James Player)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 17 2020, 4:00 PM (4 w, 4 d)

Recent Activity

Fri, Jan 15

jplayer-nv added a comment to D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

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

Fri, Jan 15, 9:40 AM · Restricted Project

Thu, Jan 14

jplayer-nv updated the diff for D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

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

Thu, Jan 14, 2:58 PM · Restricted Project
jplayer-nv updated the diff for D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

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.

Thu, Jan 14, 12:44 PM · Restricted Project
jplayer-nv added a comment to D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

Taking a look at the gcc 5.4 failure now.

Thu, Jan 14, 10:00 AM · Restricted Project

Tue, Jan 12

jplayer-nv added a comment to D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

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

Tue, Jan 12, 9:11 AM · Restricted Project

Mon, Jan 11

jplayer-nv updated the diff for D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

Properly update the patch to incorporate the separate static_asserts.

Mon, Jan 11, 2:56 PM · Restricted Project
jplayer-nv updated the diff for D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

Split conditions into individual static_asserts.

Mon, Jan 11, 2:33 PM · Restricted Project

Fri, Jan 8

jplayer-nv added a comment to D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

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

Fri, Jan 8, 4:42 PM · Restricted Project
jplayer-nv updated the diff for D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

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

Fri, Jan 8, 12:09 PM · Restricted Project

Tue, Jan 5

jplayer-nv added inline comments to D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.
Tue, Jan 5, 1:28 PM · Restricted Project

Mon, Jan 4

jplayer-nv added inline comments to D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.
Mon, Jan 4, 4:52 PM · Restricted Project
jplayer-nv added inline comments to D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.
Mon, Jan 4, 4:10 PM · Restricted Project
jplayer-nv added inline comments to D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.
Mon, Jan 4, 12:15 PM · Restricted Project
jplayer-nv updated the diff for D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.
  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.
Mon, Jan 4, 12:12 PM · Restricted Project

Dec 18 2020

jplayer-nv added inline comments to D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.
Dec 18 2020, 2:32 PM · Restricted Project
jplayer-nv updated the diff for D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

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

Dec 18 2020, 2:32 PM · Restricted Project
jplayer-nv added a comment to D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.

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.

Dec 18 2020, 11:35 AM · Restricted Project
jplayer-nv updated the diff for D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.
  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.
Dec 18 2020, 11:32 AM · Restricted Project

Dec 17 2020

jplayer-nv requested review of D93510: Fix llvm::Optional build breaks in MSVC using std::is_trivially_copyable.
Dec 17 2020, 8:56 PM · Restricted Project
jplayer-nv added a comment to D92514: Switch from llvm::is_trivially_copyable to std::is_trivially_copyable.

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?

Dec 17 2020, 5:00 PM · Restricted Project
jplayer-nv added a comment to D92514: Switch from llvm::is_trivially_copyable to std::is_trivially_copyable.

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.

Dec 17 2020, 4:43 PM · Restricted Project