[llvm] r260654 - [ADT] OptionSet: ifdef out some code that seems to be crashing MSVC.

Fri Feb 12 15:11:39 PST 2016

> On Feb 12, 2016, at 15:04, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
> 
>> 
>> On Feb 12, 2016, at 2:42 PM, Aaron Ballman <aaron at aaronballman.com <mailto:aaron at aaronballman.com>> wrote:
>> 
>> On Fri, Feb 12, 2016 at 5:36 PM, Jordan Rose <jordan_rose at apple.com <mailto:jordan_rose at apple.com>> wrote:
>>> 
>>> On Feb 12, 2016, at 14:33, Aaron Ballman <aaron at aaronballman.com <mailto:aaron at aaronballman.com>> wrote:
>>> 
>>> On Fri, Feb 12, 2016 at 4:58 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com <mailto:akyrtzi at gmail.com>>
>>> wrote:
>>> 
>>> 
>>> On Feb 12, 2016, at 1:29 PM, Aaron Ballman <aaron at aaronballman.com <mailto:aaron at aaronballman.com>> wrote:
>>> 
>>> On Fri, Feb 12, 2016 at 12:40 PM, Jordan Rose <jordan_rose at apple.com <mailto:jordan_rose at apple.com>> wrote:
>>> 
>>> 
>>> On Feb 12, 2016, at 7:58 , Argyrios Kyrtzidis <akyrtzi at gmail.com <mailto:akyrtzi at gmail.com>> wrote:
>>> 
>>> On Feb 12, 2016, at 5:08 AM, Aaron Ballman <aaron at aaronballman.com <mailto:aaron at aaronballman.com>> wrote:
>>> 
>>> On Thu, Feb 11, 2016 at 11:36 PM, Argyrios Kyrtzidis via llvm-commits
>>> <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>> 
>>> Author: akirtzidis
>>> Date: Thu Feb 11 22:36:48 2016
>>> New Revision: 260654
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=260654&view=rev <http://llvm.org/viewvc/llvm-project?rev=260654&view=rev>
>>> Log:
>>> [ADT] OptionSet: ifdef out some code that seems to be crashing MSVC.
>>> 
>>> Modified:
>>> llvm/trunk/include/llvm/ADT/OptionSet.h
>>> 
>>> Modified: llvm/trunk/include/llvm/ADT/OptionSet.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/OptionSet.h?rev=260654&r1=260653&r2=260654&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/OptionSet.h?rev=260654&r1=260653&r2=260654&view=diff>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/ADT/OptionSet.h (original)
>>> +++ llvm/trunk/include/llvm/ADT/OptionSet.h Thu Feb 11 22:36:48 2016
>>> @@ -116,6 +116,8 @@ public:
>>> }
>>> 
>>> private:
>>> +#ifndef _MSC_VER
>>> +  // This is crashing MSVC.
>>> template <typename T>
>>> static auto _checkResultTypeOperatorOr(T t) -> decltype(t | t) { return
>>> T(); }
>>> 
>>> @@ -124,6 +126,7 @@ private:
>>> static_assert(!std::is_same<decltype(_checkResultTypeOperatorOr(Flags())),
>>>                           Flags>::value,
>>>             "operator| should produce an OptionSet");
>>> +#endif
>>> };
>>> 
>>> 
>>> Are there plans to correct this for MSVC and recommit? Is there a
>>> requirement for this to use automatic type deduction instead of using
>>> the decltype in the is_same check, like this:
>>> 
>>> http://coliru.stacked-crooked.com/a/94f224987d9feaee
>>> 
>>> 
>>> + Jordan who added that.
>>> 
>>> 
>>> That seems perfectly fine to me—actually, it seems like an improvement. I
>>> don't know why I did it the way I did.
>>> 
>>> Actually, this would be an even better improvement:
>>> 
>>> static_assert(std::is_same<decltype(Flags() | Flags()),
>>>                         OptionSet<Flags>>::value,
>>>              "operator| should produce an OptionSet");
>>> 
>>> …but I'm not sure if that will break any of our existing use cases!
>>> 
>>> 
>>> It turns out that it was written that way because it had to be.
>>> OptionSetTest.cpp has a test case using enum class, for which operator
>>> | does not compile. Without SFINAE, there's no way to test that.
>>> 
>>> I am able to reproduce the compiler ICE in MSVC 2013; I will see if
>>> there's a way I can correct this so we can enable the check on Visual
>>> Studio.
>>> 
>>> 
>>> Thanks Aaron!
>>> 
>>> FYI, there were also 2 other issues on MSVC:
>>> 
>>> Compiler error in unit test:
>>> http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/9720
>>> Unit test failure:
>>> http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/9721
>>> 
>>> 
>>> I've attached a patch that addresses the static_assert issue and
>>> updates some comments. As for the testing issues, those will have to
>>> wait until MSVC 2013 support is dropped. The first issue is because
>>> MSVC 2013 does not have constexpr support for std::numeric_limits. The
>>> second issue is because is_constructible returns true despite
>>> sizeof(intptr_t) == 4 and sizeof(long long) == 8. I don't have the
>>> chance to look into the second one to see whether the test is
>>> incorrect, or whether MSVC's implementation of std::is_constructible
>>> is incorrect right now, but I think it's fine to leave the unit test
>>> out for MSVC currently.
>>> 
>>> 
>>> This patch is no good because it doesn't catch what the static_assert is
>>> actually checking for, which is
>>> 
>>> enum Flags { A = 1, B = 2 };
>>> Flags operator|(Flags lhs, Flags rhs) {
>>> return static_cast<Flags>(lhs | rhs);
>>> }
>>> 
>>> OptionSet<Flags> myFlags = A | B;
>>> 
>>> 
>>> The idea is that if you use OptionSet<Flags>, operator| should always give
>>> you an OptionSet, not the raw type.
>> 
>> Good catch! It's strange that this does not cause any test failures,
>> however. Unfortunately, I don't have any more time I can dedicate to
>> this right now, but it gives Argyrios an idea of how to modify the
>> code to compile for MSVC 2013 without causing an ICE. I'll let them
>> take care of actually correcting it properly -- I am happy to test any
>> proposed patches. Another thread is recommending that this entire file
>> be reverted and go through the review process, so this is something
>> that we can catch at that time. Given that this is exercising
>> questionable corners of MSVC, we definitely should not have this
>> static_assert commented out if we can at all avoid it.
> 
> I’m not familiar with this code, Jordan, what can go wrong if the static_assert is ifdef’ed out ? 

Well, nothing, really; it's an enforced style check. Here's the original commit <https://github.com/apple/swift/commit/6cf9576a0505344221ba5b530aff0d214ffac61a>, and it's explicitly marked NFC.

Jordan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160212/3acf6d37/attachment.html>