[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 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.

~Aaron

>
> Jordan
>
> P.S. Argyrios, please switch the uses of swift::OptionSet over to
> llvm::OptionSet at some point.