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

[Jordan Rose via llvm-commits]

> On Feb 12, 2016, at 14:33, Aaron Ballman <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> wrote:
>> 
>> On Fri, Feb 12, 2016 at 12:40 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>> 
>> 
>> On Feb 12, 2016, at 7:58 , Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>> 
>> On Feb 12, 2016, at 5:08 AM, Aaron Ballman <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> 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
>> 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
>> ==============================================================================
>> --- 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.

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