Bug 191146

(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.

<rdar://problem/45802019>