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

[Aaron Ballman via llvm-commits]

On Fri, Feb 12, 2016 at 4:58 PM, Argyrios Kyrtzidis <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.

~Aaron

>
>
> ~Aaron
>
>
> Jordan
>
> P.S. Argyrios, please switch the uses of swift::OptionSet over to
> llvm::OptionSet at some point.
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: adt.patch
Type: application/octet-stream
Size: 1803 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160212/bb6c3e19/attachment.obj>