Bug 191146 – [Win][Clang][JSC] JIT::is64BitType reports "warning: explicit specialization cannot have a storage class"

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Fujii Hironori]

[Win][Clang][JSC] JIT::is64BitType reports "warning: explicit specialization cannot have a storage class"

clang-cl 7.0 reports following compilation error.

> In file included from DerivedSources\JavaScriptCore\unified-sources\UnifiedSource26.cpp:2:
> In file included from ..\..\Source\JavaScriptCore\bytecode/CodeBlock.cpp:59:
> ..\..\Source\JavaScriptCore\jit\JIT.h(787,31):  warning: explicit specialization cannot have a storage class
>         static constexpr bool is64BitType<void>() { return true; }
>         ~~~~~~~               ^
> 1 warning generated.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #0)
> clang-cl 7.0 reports following compilation error.

Oops. It is a warning, not error.

[Fujii Hironori]

c++ - constexpr static template function: g++ error is a warning on clang - Stack Overflow
https://stackoverflow.com/q/37879642/2691131

[Fujii Hironori]

Created attachment 353588 [details]
Patch

[Ross Kirsling]

Comment on attachment 353588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353588&action=review

Looks like this one's my fault (bug 183655).

> Source/JavaScriptCore/jit/JIT.h:791
>          template<typename Type>
> -        static constexpr bool is64BitType() { return sizeof(Type) <= 8; }
> +        struct is64BitType {
> +            static const bool value = sizeof(Type) <= 8;
> +        };
>  
>          template<>
> -        static constexpr bool is64BitType<void>() { return true; }
> +        struct is64BitType<void> {
> +            static const bool value = true;
> +        };

Could we drop `static` and move this to an anonymous namespace instead?
It probably shouldn't have been a member to begin with... 😓

[Fujii Hironori]

Comment on attachment 353588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353588&action=review

>> Source/JavaScriptCore/jit/JIT.h:791
>> +        };
> 
> Could we drop `static` and move this to an anonymous namespace instead?
> It probably shouldn't have been a member to begin with... 😓

I tested both patches with both compilers, and confirmed both
works fine. I prefer the inner template class version to template
class version because anonymous namespace is useless in Unified
Source Builds.

C++14 has template variable. I will refine the patch.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #5)
> C++14 has template variable. I will refine the patch.

Oh, no. I am hitting the following issue if I use a template variable.

  variable template causes internal compiler error when used in a static_assert inside class template - Developer Community
  https://developercommunity.visualstudio.com/content/problem/268619/variable-template-causes-internal-compiler-error-w.html

This is fixed in Visual Studio 2017 version 15.8.

[Fujii Hironori]

Comment on attachment 353588 [details]
Patch

Review, please.

[Keith Miller]

Comment on attachment 353588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353588&action=review

>>> Source/JavaScriptCore/jit/JIT.h:791
>>> +        };
>> 
>> Could we drop `static` and move this to an anonymous namespace instead?
>> It probably shouldn't have been a member to begin with... 😓
> 
> I tested both patches with both compilers, and confirmed both
> works fine. I prefer the inner template class version to template
> class version because anonymous namespace is useless in Unified
> Source Builds.
> 
> C++14 has template variable. I will refine the patch.

Does anyone actually use the fact that is64BitType is static? Can we just removed that keyword?

[Fujii Hironori]

Comment on attachment 353588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353588&action=review

>>>> Source/JavaScriptCore/jit/JIT.h:791
>>>> +        };
>>> 
>>> Could we drop `static` and move this to an anonymous namespace instead?
>>> It probably shouldn't have been a member to begin with... 😓
>> 
>> I tested both patches with both compilers, and confirmed both
>> works fine. I prefer the inner template class version to template
>> class version because anonymous namespace is useless in Unified
>> Source Builds.
>> 
>> C++14 has template variable. I will refine the patch.
> 
> Does anyone actually use the fact that is64BitType is static? Can we just removed that keyword?

It can't compile by GCC, Clang, MSVC if is64BitType is non-static member function.
Maybe, 'this' can not be used in static_assert.
https://godbolt.org/z/NxG5HI

[Ross Kirsling]

(In reply to Fujii Hironori from comment #9)
> Comment on attachment 353588 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353588&action=review
> 
> >>>> Source/JavaScriptCore/jit/JIT.h:791
> >>>> +        };
> >>> 
> >>> Could we drop `static` and move this to an anonymous namespace instead?
> >>> It probably shouldn't have been a member to begin with... 😓
> >> 
> >> I tested both patches with both compilers, and confirmed both
> >> works fine. I prefer the inner template class version to template
> >> class version because anonymous namespace is useless in Unified
> >> Source Builds.
> >> 
> >> C++14 has template variable. I will refine the patch.
> > 
> > Does anyone actually use the fact that is64BitType is static? Can we just removed that keyword?
> 
> It can't compile by GCC, Clang, MSVC if is64BitType is non-static member
> function.
> Maybe, 'this' can not be used in static_assert.
> https://godbolt.org/z/NxG5HI

As I mentioned, it was my mistake. It's just a simple helper function that doesn't need to be a member at all, but I must've guessed that it would better align with existing practices to make it a method instead of namespace-static. But as you've noted, simply removing `static` won't compile.

[Yusuke Suzuki]

Comment on attachment 353588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=353588&action=review

OK, then r=me

> Source/JavaScriptCore/jit/JIT.h:785
> +            static const bool value = sizeof(Type) <= 8;

Ditto.

> Source/JavaScriptCore/jit/JIT.h:790
> +            static const bool value = true;

Use `static constexpr bool`

>>>>>> Source/JavaScriptCore/jit/JIT.h:791
>>>>>> +        };
>>>>> 
>>>>> Could we drop `static` and move this to an anonymous namespace instead?
>>>>> It probably shouldn't have been a member to begin with... 😓
>>>> 
>>>> I tested both patches with both compilers, and confirmed both
>>>> works fine. I prefer the inner template class version to template
>>>> class version because anonymous namespace is useless in Unified
>>>> Source Builds.
>>>> 
>>>> C++14 has template variable. I will refine the patch.
>>> 
>>> Does anyone actually use the fact that is64BitType is static? Can we just removed that keyword?
>> 
>> It can't compile by GCC, Clang, MSVC if is64BitType is non-static member function.
>> Maybe, 'this' can not be used in static_assert.
>> https://godbolt.org/z/NxG5HI
> 
> As I mentioned, it was my mistake. It's just a simple helper function that doesn't need to be a member at all, but I must've guessed that it would better align with existing practices to make it a method instead of namespace-static. But as you've noted, simply removing `static` won't compile.

I'm not sure whether template variable can be used in some versions of GCC... But it may be now updated.

[Fujii Hironori]

Created attachment 353827 [details]
Patch for landing

Thank you for the review. Addressed the review feedback.

[Fujii Hironori]

Comment on attachment 353827 [details]
Patch for landing

Clearing flags on attachment: 353827

Committed r237793: <https://trac.webkit.org/changeset/237793>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45802019>