[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 5:36 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>
> 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>
> 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.

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.

~Aaron