[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 6:04 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>
> On Feb 12, 2016, at 2:42 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>
> 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.
>
>
> I’m not familiar with this code, Jordan, what can go wrong if the
> static_assert is ifdef’ed out ?
> It seems to me this is exercising corners of MSVC in trying to do SFINAE in
> the static assert, but the class itself and using it is not complicated.
>
> Aaron, if MSVC 2015 is fine in all aspects, how about ifdef’ing out
> specifically for MSVC 2013, how can we do that ?

Include compiler.h and use LLVM_MSC_PREREQ(1900). I think it would be
totally fine to enable the checks just for MSVC 2015, and not 2013. I
was only worried about not enabling the checks for MSVC at all. If we
can also support 2013 with the static_assert, that would be best, of
course.

~Aaron

>
>
> ~Aaron
>
>