Page MenuHomePhabricator

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

Authored by jplayer-nv on Thu, Dec 17, 8:56 PM.

Details

Summary

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jplayer-nv requested review of this revision.Thu, Dec 17, 8:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Dec 17, 8:56 PM

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.

Would it be possible to add a test case for the is_trivially_copy_constructible case you speculated about?

llvm/include/llvm/ADT/Optional.h
36–60

Could you run this through clang-format?

jplayer-nv edited the summary of this revision. (Show Details)
  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.

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.

Likely related to a recent change to std::pair in MSVC. They added the following:

pair& operator=(const volatile pair&) = delete;

This seems to implicitly delete the default version of the copy assignment. Although, they do have a non-trivial template which serves as the copy assignment in place the the deleted default one. Not sure whether MS intended to delete the default copy assignment here, but this is valid C++ code as far as I can tell.

The test cases I added to OptionalTest.cpp should replicate these conditions for both the copy assignment and the copy constructor.

llvm/include/llvm/ADT/Optional.h
36–60

This was already run through clang-format.

dblaikie added inline comments.Fri, Dec 18, 1:43 PM
llvm/include/llvm/ADT/Optional.h
36–59

Is the "is_trivially_copyable" useful/necessary anymore? Would it be better to remove it (perhaps we need "is_trivially_destructible" in its place)?

llvm/unittests/ADT/OptionalTest.cpp
399

This comment might be too literal (it doesn't seem to add a lot compared to reading the line of source code) - maybe an explanation of why deleting the volatile copy constructor is important/relevant would be helpful?

(similar in the other class)

412–413

Could you throw some static assertions in here to match the comment for this class and show that it's trivially copyable but not trivially copy constructible?

(similar for the other class)

jplayer-nv marked 2 inline comments as done.

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

llvm/include/llvm/ADT/Optional.h
36–59

Yes I think it is. It covers the is_trivially_destructible conditon, but it also covers non-trivial move assignment / constructor. Pretty sure in the case of a deleted move constructor / assignment, we would fall back to the copy constructor / assignment (so current conditions should be sufficient to avoid build break). At the same time, we want to honor any non-trivial move assignment / constructor that may exist.

llvm/unittests/ADT/OptionalTest.cpp
412–413

I thought about that but deleted them for fear of breaking a build. With such a wide array of compilers building this code, one is bound to come up with an unexpected result.

I agree it would make the code easier to read, but I still don't think it's a good idea to incorporate into the commit. My thought was that we're testing llvm::Optional here, not the host compiler.

If you feel strongly, I will add them back.

dblaikie added inline comments.Tue, Dec 22, 2:54 PM
llvm/include/llvm/ADT/Optional.h
36–59

Fair enough - so what would the fully expanded check look like? is_trivially_copy_constructible + is_trivially_copy_assignable + is_trivially_move_constructible + is_trivially_move_assignable + is_trivially_destructible? That might be more clear than as its written, even though it's more verbose.

36–60

Ah, clang-format is getting confused about whether these are types or values. If you put some () around the whole expression (bool = (x && y && z)) it tidies this up a lot.

llvm/unittests/ADT/OptionalTest.cpp
412–413

I'd prefer to add it, and remove it with a comment if it does break something, so we know what we're working around and why.

Yeah, we aren't testing the host compiler - but it's pretty non-trivial how this type produces the specific features that are interesting to the test, so validating that those features have been created as intended seems valuable to me. Especially if there's a good chance a compiler might not be implementing them correctly & thus could lead to confusing test behavior, etc.

jplayer-nv updated this revision to Diff 314429.EditedMon, Jan 4, 12:12 PM
  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.
  4. Added requests static_asserts to validate class properties in Optional testing code.
jplayer-nv marked 5 inline comments as done.Mon, Jan 4, 12:15 PM
jplayer-nv added inline comments.
llvm/include/llvm/ADT/Optional.h
36–59

You don't need a class to be trivially movable or trivially move assignable in order to use the trivial OptionalStorage specialization. The move constructor / assignment can be deleted, and the compiler will fall back to the copy constructor / assignment.

We just need to verify that the user hasn't specified a custom move constructor / assignment. This is perfectly captured by is_trivially_copyable. That is, if either the move constructor or assignment are deleted, they will not be "eligible" in the spec, and therefore will not disqualify the class from being trivially copyable.

If we were to expand this out, I think it would look like:

   std::is_trivially_copy_constructible<T>::value
&& std::is_trivially_copy_assignable<T>::value
&& (!std::is_move_constructible<T>::value || std::is_trivially_move_constructible<T>::value)
&& (!std::is_move_assignable<T>::value || std::is_trivially_move_assignable<T>::value)
dblaikie added inline comments.Mon, Jan 4, 3:29 PM
llvm/include/llvm/ADT/Optional.h
36–59

That doesn't sound quite right to me. If something isn't move constructible or isn't move assignable, wouldn't that break the places where Optional tries to move them?

The move constructor / assignment can be deleted, and the compiler will fall back to the copy constructor / assignment.

I don't think that's quite right - if something has an explicitly deleted move constructor, it'll be "!std::is_move_constructible" and move construction expressions will fail to compile, like this: https://godbolt.org/z/36xfb7

So I think your proposed expansion is equivalent to:

   std::is_trivially_copy_constructible<T>::value
&& std::is_trivially_copy_assignable<T>::value
&& std::is_trivially_move_constructible<T>::value
&& std::is_trivially_move_assignable<T>::value

Which seems like it might be nice to have it written out that way?

jplayer-nv marked an inline comment as done.Mon, Jan 4, 4:10 PM
jplayer-nv added inline comments.
llvm/include/llvm/ADT/Optional.h
36–59

The move constructor / assignment can be deleted, and the compiler will fall back to the copy constructor / assignment.

I don't think that's quite right - if something has an explicitly deleted move constructor, it'll be "!std::is_move_constructible" and move construction expressions will fail to compile, like this: https://godbolt.org/z/36xfb7

I must be missing something, because the code example you provided seems to reinforce my intended point. I'll admit that I overstated, but it applies when dealing with a field of another class. Here's a trimmed version of the code example from your last comment:
https://godbolt.org/z/naE4PY

Even though t1 has its move constructor explicitly deleted, t2 is still trivially movable. No compile error when we try to move construct the class which contains a field with a deleted move constructor.

I did add a test to demonstrate this property of Optional. If its value_type is not movable, then compilation will not fail when the Optional is moved.

dblaikie added inline comments.Mon, Jan 4, 4:25 PM
llvm/include/llvm/ADT/Optional.h
36–59

Even though t1 has its move constructor explicitly deleted, t2 is still trivially movable.

Yep, I'm with you there - with or without the explicit defaults (in your t2 or my t2), the type is trivially movable.

No compile error when we try to move construct the class which contains a field with a deleted move constructor.

Yep, agreed.

I did add a test to demonstrate this property of Optional. If its value_type is not movable,

What do you mean by "not movable" here? Note that t2 in both our examples returns true for "is_move_constructible" despite the fact that move construction will invoke a copy constructor instead.

I think the specific thing I'm looking for (could you provide this in the form of a struct definition, preferrably in a godbolt link) is a case where this function returns false:

#include <type_traits>
template <typename T>
bool test() {
  return (std::is_trivially_copy_constructible<T>::value &&
          std::is_trivially_copy_assignable<T>::value &&
          (!std::is_move_constructible<T>::value ||
           std::is_trivially_move_constructible<T>::value) &&
          (!std::is_move_assignable<T>::value ||
           std::is_trivially_move_assignable<T>::value)) ==
         (std::is_trivially_copy_constructible<T>::value &&
          std::is_trivially_copy_assignable<T>::value &&
          std::is_trivially_move_constructible<T>::value &&
          std::is_trivially_move_assignable<T>::value);
}

If there is no such type for which that function returns false, then I think the simpler of these two expression (is_trivially_{copy,move}_{assignable,constructible}) would be the preferable/tidier/easier to read expression of the intent here.

jplayer-nv added inline comments.Mon, Jan 4, 4:51 PM
llvm/include/llvm/ADT/Optional.h
36–59

I did add a test to demonstrate this property of Optional. If its value_type is not movable,

What do you mean by "not movable" here? Note that t2 in both our examples returns true for "is_move_constructible" despite the fact that move construction will invoke a copy constructor instead.

I mean that std::is_move_constructible<Optional<Foo>::value_type>::value == false. In this case, Optional will invoke the copy constructor as long as Foo is trivially copy constructible.

I think the specific thing I'm looking for (could you provide this in the form of a struct definition, preferrably in a godbolt link) is a case where this function returns false:

Sure thing.
https://godbolt.org/z/ccxj6n

The simpler condition is unnecessarily conservative when the move constructor / assignment are deleted.

llvm/unittests/ADT/OptionalTest.cpp
412–413

Looks like the static asserts are firing. So will comment them out when we converge on the OptionalStorage specialization condition.

dblaikie added inline comments.Tue, Jan 5, 1:09 PM
llvm/include/llvm/ADT/Optional.h
36–59

I mean that std::is_move_constructible<Optional<Foo>::value_type>::value == false. In this case, Optional will invoke the copy constructor as long as Foo is trivially copy constructible.

OK, good to understand - thanks for helping me through it. Sorry this is all a bit of a slog.

Sure thing.
https://godbolt.org/z/ccxj6n

Ah, thanks!

The simpler condition is unnecessarily conservative when the move constructor / assignment are deleted.

Fair enough - not sure it's necessarily an important entity to optimize for (trivially copyable, but deleted move op is a bit weird), but glad to understand/OK with the direction you're proposing here.

llvm/unittests/ADT/OptionalTest.cpp
412–413

Firing in some pre-merge checks? Got some links/details on the failures?

jplayer-nv added inline comments.Tue, Jan 5, 1:28 PM
llvm/include/llvm/ADT/Optional.h
36–59

No worries. This is all very pedantic.

llvm/unittests/ADT/OptionalTest.cpp
412–413

Correct. Windows passed, but debian and clang-tidy both failed on the static_asserts:

Windows
Debian
clang-tidy

jplayer-nv added a reviewer: dblaikie.

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

jplayer-nv marked 4 inline comments as done.Fri, Jan 8, 4:42 PM

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

dblaikie accepted this revision.Fri, Jan 8, 9:36 PM

Looks good - thanks!

llvm/unittests/ADT/OptionalTest.cpp
417–421

Probably best to split that into two static_asserts, so it's easier to diagnose failures.

This revision is now accepted and ready to land.Fri, Jan 8, 9:36 PM

Split conditions into individual static_asserts.

jplayer-nv marked an inline comment as done.

Properly update the patch to incorporate the separate static_asserts.

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

aganea added a subscriber: aganea.Wed, Jan 13, 8:07 PM
aganea added inline comments.
llvm/unittests/ADT/OptionalTest.cpp
416

When compiling with clang-cl 11.0 into a "x64 Native Tools Command Prompt for VS 2019" (16.8.4) cmd.exe, I see:

F:\aganea\llvm-project\llvm\unittests\ADT\OptionalTest.cpp(417,1): error: static_assert failed due to requirement
      'std::is_trivially_copyable<(anonymous namespace)::NonTCopy>::value' "Expect NonTCopy to be trivially copyable"
static_assert(std::is_trivially_copyable<NonTCopy>::value,
^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
F:\aganea\llvm-project\llvm\unittests\ADT\OptionalTest.cpp(465,1): error: static_assert failed due to requirement
      'std::is_trivially_copyable<(anonymous namespace)::NonTAssign>::value' "Expect NonTAssign to be trivially copyable"
static_assert(std::is_trivially_copyable<NonTAssign>::value,
^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.

This is because Clang's -fms-compatibility-version inherits its version from the cl.exe in %PATH%.

I've tried with Clang 11.0 and Clang 12.0 at HEAD (rGeec856848ccc481b2704ebf64d725e635a3d7dca). They both insist that NonTCopy isn't __is_trivially_copyable but MSVC 16.8.4 and GCC at HEAD says it is.

dblaikie added inline comments.Wed, Jan 13, 8:51 PM
llvm/unittests/ADT/OptionalTest.cpp
416

Since I don't have a setup for this - would you mind testing (& if it works, committing) adding a && !defined(__clang__) to these #ifs?

@jplayer-nv It looks like this patch has broken the ppc buildbots - please can you take a look? http://lab.llvm.org:8011/#/builders/93/builds/1435

It seems that this patch breaks clangd build with GCC 5.4 (reverting this patch fixes the build)

[2011/2198] Building CXX object tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/CodeComplete.cpp.o
FAILED: /usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/tools/extra/clangd -I/mnt/d/work/llvm-project/clang-tools-extra/clangd -Itools/clang/tools/extra/clangd/../clang-tidy -I/mnt/d/work/llvm-project/clang/include -Itools/clang/include -Iinclude -I/mnt/d/work/llvm-project/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3 -DNDEBUG  -fno-exceptions -fno-rtti -std=c++14 -MD -MT tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/CodeComplete.cpp.o -MF tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/CodeComplete.cpp.o.d -o tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/CodeComplete.cpp.o -c /mnt/d/work/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp
In file included from /mnt/d/work/llvm-project/llvm/include/llvm/ADT/StringMap.h:16:0,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/CompileCommands.h:14,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/GlobalCompilationDatabase.h:12,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/Compiler.h:18,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/CodeComplete.h:18,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:20:
/mnt/d/work/llvm-project/llvm/include/llvm/ADT/StringMapEntry.h: In instantiation of ‘llvm::StringMapEntryStorage<ValueTy>::StringMapEntryStorage(size_t, InitTy&& ...) [with InitTy = {std::unique_ptr<clang::clangd::FileDistance, std::default_delete<clang::clangd::FileDistance> >&}; ValueTy = std::unique_ptr<clang::clangd::FileDistance>; size_t = long unsigned int]’:
/mnt/d/work/llvm-project/llvm/include/llvm/ADT/StringMapEntry.h:71:41:   required from ‘static llvm::StringMapEntry<ValueTy>* llvm::StringMapEntry<ValueTy>::Create(llvm::StringRef, AllocatorTy&, InitTy&& ...) [with AllocatorTy = llvm::MallocAllocator; InitTy = {std::unique_ptr<clang::clangd::FileDistance, std::default_delete<clang::clangd::FileDistance> >&}; ValueTy = std::unique_ptr<clang::clangd::FileDistance>]’
/mnt/d/work/llvm-project/llvm/include/llvm/ADT/StringMap.h:158:39:   required from ‘llvm::StringMap<ValueTy, AllocatorTy>::StringMap(const llvm::StringMap<ValueTy, AllocatorTy>&) [with ValueTy = std::unique_ptr<clang::clangd::FileDistance>; AllocatorTy = llvm::MallocAllocator]’
/mnt/d/work/llvm-project/clang-tools-extra/clangd/FileDistance.h:93:7:   required from ‘struct std::is_trivially_copy_constructible<clang::clangd::URIDistance>’
/mnt/d/work/llvm-project/llvm/include/llvm/ADT/Optional.h:36:78:   required from ‘class llvm::Optional<clang::clangd::URIDistance>’
/mnt/d/work/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:1272:31:   required from here
/mnt/d/work/llvm-project/llvm/include/llvm/ADT/StringMapEntry.h:47:49: error: use of deleted function ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = clang::clangd::FileDistance; _Dp = std::default_delete<clang::clangd::FileDistance>]’
         second(std::forward<InitTy>(initVals)...) {}
                                                 ^
In file included from /usr/include/c++/5/memory:81:0,
                 from /mnt/d/work/llvm-project/llvm/include/llvm/ADT/Optional.h:23,
                 from /mnt/d/work/llvm-project/llvm/include/llvm/ADT/STLExtras.h:19,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/support/Context.h:17,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/support/Threading.h:12,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/CompileCommands.h:11,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/GlobalCompilationDatabase.h:12,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/Compiler.h:18,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/CodeComplete.h:18,
                 from /mnt/d/work/llvm-project/clang-tools-extra/clangd/CodeComplete.cpp:20:
/usr/include/c++/5/bits/unique_ptr.h:356:7: note: declared here
       unique_ptr(const unique_ptr&) = delete;
aganea added a subscriber: rsmith.Thu, Jan 14, 5:41 AM
aganea added inline comments.
llvm/unittests/ADT/OptionalTest.cpp
416

I've reverted the patch since I think it needs a bit more testing.

@dblaikie @rsmith Do you think the NonTCopy test case above requires fixes in Clang? (so that __is_trivially_copyable returns true like the other compilers)

Taking a look at the gcc 5.4 failure now.

Now that the patch has been committed & reverted, do I start a new patch when fixes for identified issues are incorporated?

dblaikie reopened this revision.Thu, Jan 14, 10:06 AM

Taking a look at the gcc 5.4 failure now.

Now that the patch has been committed & reverted, do I start a new patch when fixes for identified issues are incorporated?

Fine to reopen and reuse this one.

This revision is now accepted and ready to land.Thu, Jan 14, 10:06 AM
dblaikie requested changes to this revision.Thu, Jan 14, 10:06 AM
This revision now requires changes to proceed.Thu, Jan 14, 10:06 AM

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.

This bug exists all the way up through gcc 7.3. Here's a godbolt link which reproduces the problem:
https://godbolt.org/z/ETW7f1

If we use llvm::is_trivially_copy_constructible<T> instead of std::is_trivially_copy_constructible<T>, then everything appears to be fine.

Interestingly, std::is_trivially_copy_assignable<T> appears not to instantiate the copy assignment because the compiler isn't complaining about its use. So I'm changing the OptionalStorage specialization condition to:

template <typename T, bool = (is_trivially_copy_constructible<T>::value &&
                              std::is_trivially_copy_assignable<T>::value &&
                              (std::is_trivially_move_constructible<T>::value ||
                               !std::is_move_constructible<T>::value) &&
                              (std::is_trivially_move_assignable<T>::value ||
                               !std::is_move_assignable<T>::value))>

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.

This bug exists all the way up through gcc 7.3. Here's a godbolt link which reproduces the problem:
https://godbolt.org/z/ETW7f1

If we use llvm::is_trivially_copy_constructible<T> instead of std::is_trivially_copy_constructible<T>, then everything appears to be fine.

Interestingly, std::is_trivially_copy_assignable<T> appears not to instantiate the copy assignment because the compiler isn't complaining about its use. So I'm changing the OptionalStorage specialization condition to:

template <typename T, bool = (is_trivially_copy_constructible<T>::value &&
                              std::is_trivially_copy_assignable<T>::value &&
                              (std::is_trivially_move_constructible<T>::value ||
                               !std::is_move_constructible<T>::value) &&
                              (std::is_trivially_move_assignable<T>::value ||
                               !std::is_move_assignable<T>::value))>

Probably worth a big comment explaining all the quirks here in the code for future readers & I'd probably suggest qualifying the unqualified one as llvm:: even though that's redundant, to make it clearer that it's intentional and not just a missed 'std::'

aganea added inline comments.Thu, Jan 14, 12:58 PM
llvm/unittests/ADT/OptionalTest.cpp
416

@jplayer-nv Could you please also add && !defined(__clang__) like suggested by David above? Same thing below at L464.

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

dblaikie accepted this revision.Thu, Jan 14, 3:02 PM

Looks worth another go

This revision is now accepted and ready to land.Thu, Jan 14, 3:02 PM
jplayer-nv marked 4 inline comments as done.Fri, Jan 15, 9:40 AM

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

I can do that. Let me test the patch on my end and I'll commit.