[webkit-reviews] review granted: [Bug 191146] [Win][Clang][JSC] JIT::is64BitType reports "warning: explicit specialization cannot have a storage class" : [Attachment 353588] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <yusukesuzuki at slowstart.org> has granted Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 191146: [Win][Clang][JSC] JIT::is64BitType reports "warning: explicit
specialization cannot have a storage class"
https://bugs.webkit.org/show_bug.cgi?id=191146

Attachment 353588: Patch

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




--- Comment #11 from Yusuke Suzuki <yusukesuzuki at slowstart.org> ---
Comment on attachment 353588
  --> https://bugs.webkit.org/attachment.cgi?id=353588
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.