| Summary: | [Win][Clang][JSC] JIT::is64BitType reports "warning: explicit specialization cannot have a storage class" | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||
| Component: | JavaScriptCore | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, saam, webkit-bug-importer, ysuzuki | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 171618 | ||||||||
| Attachments: |
|
||||||||
(In reply to Fujii Hironori from comment #0) > clang-cl 7.0 reports following compilation error. Oops. It is a warning, not error. c++ - constexpr static template function: g++ error is a warning on clang - Stack Overflow https://stackoverflow.com/q/37879642/2691131 Created attachment 353588 [details]
Patch
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... 😓 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. (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. Comment on attachment 353588 [details]
Patch
Review, please.
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? 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 (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. 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. Created attachment 353827 [details]
Patch for landing
Thank you for the review. Addressed the review feedback.
Comment on attachment 353827 [details] Patch for landing Clearing flags on attachment: 353827 Committed r237793: <https://trac.webkit.org/changeset/237793> All reviewed patches have been landed. Closing bug. |
[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.