191146 2018-11-01 00:40:37 -0700 [Win][Clang][JSC] JIT::is64BitType reports "warning: explicit specialization cannot have a storage class" 2018-11-05 01:35:19 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 171618 1 fujii fujii ews-watchlist keith_miller mark.lam msaboff ross.kirsling saam webkit-bug-importer ysuzuki oldest_to_newest 1474042 0 fujii 2018-11-01 00:40:37 -0700 [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. 1474043 1 fujii 2018-11-01 00:51:47 -0700 (In reply to Fujii Hironori from comment #0) > clang-cl 7.0 reports following compilation error. Oops. It is a warning, not error. 1474044 2 fujii 2018-11-01 01:27:16 -0700 c++ - constexpr static template function: g++ error is a warning on clang - Stack Overflow https://stackoverflow.com/q/37879642/2691131 1474053 3 353588 fujii 2018-11-01 02:22:20 -0700 Created attachment 353588 Patch 1474156 4 353588 ross.kirsling 2018-11-01 10:30:09 -0700 Comment on attachment 353588 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... 😓 1474367 5 353588 fujii 2018-11-01 18:30:06 -0700 Comment on attachment 353588 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. 1474374 6 fujii 2018-11-01 19:15:47 -0700 (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. 1474377 7 353588 fujii 2018-11-01 19:18:24 -0700 Comment on attachment 353588 Patch Review, please. 1474378 8 353588 keith_miller 2018-11-01 19:27:53 -0700 Comment on attachment 353588 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? 1474380 9 353588 fujii 2018-11-01 20:00:33 -0700 Comment on attachment 353588 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 1474393 10 ross.kirsling 2018-11-01 21:18:19 -0700 (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. 1474723 11 353588 ysuzuki 2018-11-02 19:58:54 -0700 Comment on attachment 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. 1474955 12 353827 fujii 2018-11-04 21:40:18 -0800 Created attachment 353827 Patch for landing Thank you for the review. Addressed the review feedback. 1474991 13 353827 fujii 2018-11-05 01:34:58 -0800 Comment on attachment 353827 Patch for landing Clearing flags on attachment: 353827 Committed r237793: <https://trac.webkit.org/changeset/237793> 1474992 14 fujii 2018-11-05 01:35:01 -0800 All reviewed patches have been landed. Closing bug. 1474993 15 webkit-bug-importer 2018-11-05 01:35:19 -0800 <rdar://problem/45802019> 353588 2018-11-01 02:22:20 -0700 2018-11-04 21:40:15 -0800 Patch bug-191146-20181101182220.patch text/plain 2254 fujii U3VidmVyc2lvbiBSZXZpc2lvbjogMjM3NjcwCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA2 MWRiOTNiY2FhM2M1MjYxNTNkOGY5ZGU1OWVjZGQ1NWEzZjM5YzdmLi5kYzhjMGI4ODFkNTE5YjZk OWNkZTk4YzcxMjk5YTRhYzE4OWM3MTc5IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxMyBAQAorMjAxOC0xMS0wMSAgRnVqaWkgSGlyb25vcmkgIDxIaXJvbm9yaS5GdWppaUBz b255LmNvbT4KKworICAgICAgICBbV2luXVtDbGFuZ11bSlNDXSBKSVQ6OmlzNjRCaXRUeXBlIHJl cG9ydHMgIndhcm5pbmc6IGV4cGxpY2l0IHNwZWNpYWxpemF0aW9uIGNhbm5vdCBoYXZlIGEgc3Rv cmFnZSBjbGFzcyIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE5MTE0NgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgICogaml0L0pJVC5oOiBDaGFuZ2VkIGlzNjRCaXRUeXBlIGZyb20gYSB0ZW1wbGF0ZSBjbGFz cyBtZXRob2QgdG8gYQorICAgICAgICB0ZW1wbGF0ZSBpbm5lciBjbGFzcy4KKwogMjAxOC0xMC0z MSAgRGV2aW4gUm91c3NvICA8ZHJvdXNzb0BhcHBsZS5jb20+CiAKICAgICAgICAgV2ViIEluc3Bl Y3RvcjogQ2FudmFzOiBjcmVhdGUgYSBzZXR0aW5nIGZvciBhdXRvLXJlY29yZGluZyBuZXdseSBj cmVhdGVkIGNvbnRleHRzCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L0pJ VC5oIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2ppdC9KSVQuaAppbmRleCA5ZGIxNmQ5M2M5NDQ0 YjUwMzBmY2U5N2M1MTBjMDZiNGE1Yjg5NjBmLi5jNmQ3Y2U4OTFjZTRiYmJkYTk1OTBjOGE2ZTA3 N2FkMTEzZTZmMWQ1IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvaml0L0pJVC5o CisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvSklULmgKQEAgLTc4MSwxNiArNzgxLDIw IEBAIG5hbWVzcGFjZSBKU0MgewogICAgICAgICB9CiAKICAgICAgICAgdGVtcGxhdGU8dHlwZW5h bWUgVHlwZT4KLSAgICAgICAgc3RhdGljIGNvbnN0ZXhwciBib29sIGlzNjRCaXRUeXBlKCkgeyBy ZXR1cm4gc2l6ZW9mKFR5cGUpIDw9IDg7IH0KKyAgICAgICAgc3RydWN0IGlzNjRCaXRUeXBlIHsK KyAgICAgICAgICAgIHN0YXRpYyBjb25zdCBib29sIHZhbHVlID0gc2l6ZW9mKFR5cGUpIDw9IDg7 CisgICAgICAgIH07CiAKICAgICAgICAgdGVtcGxhdGU8PgotICAgICAgICBzdGF0aWMgY29uc3Rl eHByIGJvb2wgaXM2NEJpdFR5cGU8dm9pZD4oKSB7IHJldHVybiB0cnVlOyB9CisgICAgICAgIHN0 cnVjdCBpczY0Qml0VHlwZTx2b2lkPiB7CisgICAgICAgICAgICBzdGF0aWMgY29uc3QgYm9vbCB2 YWx1ZSA9IHRydWU7CisgICAgICAgIH07CiAKICAgICAgICAgdGVtcGxhdGU8dHlwZW5hbWUgT3Bl cmF0aW9uVHlwZSwgdHlwZW5hbWUuLi4gQXJncz4KICAgICAgICAgc3RkOjplbmFibGVfaWZfdDwh c3RkOjppc19zYW1lPHR5cGVuYW1lIEZ1bmN0aW9uVHJhaXRzPE9wZXJhdGlvblR5cGU+OjpSZXN1 bHRUeXBlLCBTbG93UGF0aFJldHVyblR5cGU+Ojp2YWx1ZSwgTWFjcm9Bc3NlbWJsZXI6OkNhbGw+ CiAgICAgICAgIGNhbGxPcGVyYXRpb24oT3BlcmF0aW9uVHlwZSBvcGVyYXRpb24sIEFyZ3MuLi4g YXJncykKICAgICAgICAgewotICAgICAgICAgICAgc3RhdGljX2Fzc2VydChpczY0Qml0VHlwZTx0 eXBlbmFtZSBGdW5jdGlvblRyYWl0czxPcGVyYXRpb25UeXBlPjo6UmVzdWx0VHlwZT4oKSwgIldp bjY0IGNhbm5vdCB1c2Ugc3RhbmRhcmQgY2FsbCB3aGVuIHJldHVybiB0eXBlIGlzIGxhcmdlciB0 aGFuIDY0IGJpdHMuIik7CisgICAgICAgICAgICBzdGF0aWNfYXNzZXJ0KGlzNjRCaXRUeXBlPHR5 cGVuYW1lIEZ1bmN0aW9uVHJhaXRzPE9wZXJhdGlvblR5cGU+OjpSZXN1bHRUeXBlPjo6dmFsdWUs ICJXaW42NCBjYW5ub3QgdXNlIHN0YW5kYXJkIGNhbGwgd2hlbiByZXR1cm4gdHlwZSBpcyBsYXJn ZXIgdGhhbiA2NCBiaXRzLiIpOwogICAgICAgICAgICAgc2V0dXBBcmd1bWVudHM8T3BlcmF0aW9u VHlwZT4oYXJncy4uLik7CiAgICAgICAgICAgICByZXR1cm4gYXBwZW5kQ2FsbFdpdGhFeGNlcHRp b25DaGVjayhvcGVyYXRpb24pOwogICAgICAgICB9Cg== 353827 2018-11-04 21:40:18 -0800 2018-11-04 21:40:18 -0800 Patch for landing bug-191146-20181105144016.patch text/plain 2240 fujii U3VidmVyc2lvbiBSZXZpc2lvbjogMjM3Nzg5CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBj ZDYwNzAwOGY0ODEwYjY0YzUzZDBjYzUxNGJhMjU4OTkxZDFhNDFiLi43N2VmY2QyNmU4NzBjYzI1 ZWY0OGY4ODY5NmYyZGQwZDBiOWEzMGM4IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxMyBAQAorMjAxOC0xMS0wNCAgRnVqaWkgSGlyb25vcmkgIDxIaXJvbm9yaS5GdWppaUBz b255LmNvbT4KKworICAgICAgICBbV2luXVtDbGFuZ11bSlNDXSBKSVQ6OmlzNjRCaXRUeXBlIHJl cG9ydHMgIndhcm5pbmc6IGV4cGxpY2l0IHNwZWNpYWxpemF0aW9uIGNhbm5vdCBoYXZlIGEgc3Rv cmFnZSBjbGFzcyIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE5MTE0NgorCisgICAgICAgIFJldmlld2VkIGJ5IFl1c3VrZSBTdXp1a2kuCisKKyAgICAg ICAgKiBqaXQvSklULmg6IENoYW5nZWQgaXM2NEJpdFR5cGUgZnJvbSBhIHRlbXBsYXRlIGNsYXNz IG1ldGhvZCB0byBhCisgICAgICAgIHRlbXBsYXRlIGlubmVyIGNsYXNzLgorCiAyMDE4LTExLTAy ICBLZWl0aCBNaWxsZXIgIDxrZWl0aF9taWxsZXJAYXBwbGUuY29tPgogCiAgICAgICAgIEFzc2Vy dCBKU1ZhbHVlcyBjYW4gZml0IGludG8gYSBwb2ludGVyIHdoZW4gQVBJIGNhc3RpbmcKZGlmZiAt LWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvSklULmggYi9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvaml0L0pJVC5oCmluZGV4IDlkYjE2ZDkzYzk0NDRiNTAzMGZjZTk3YzUxMGMwNmI0YTVi ODk2MGYuLjZkYjRkN2Y2YWRlM2FlZDQ2MDhkMDIyMzU2MTBlNzNkMDVkNWNlOTcgMTAwNjQ0Ci0t LSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9qaXQvSklULmgKKysrIGIvU291cmNlL0phdmFTY3Jp cHRDb3JlL2ppdC9KSVQuaApAQCAtNzgxLDE2ICs3ODEsMjAgQEAgbmFtZXNwYWNlIEpTQyB7CiAg ICAgICAgIH0KIAogICAgICAgICB0ZW1wbGF0ZTx0eXBlbmFtZSBUeXBlPgotICAgICAgICBzdGF0 aWMgY29uc3RleHByIGJvb2wgaXM2NEJpdFR5cGUoKSB7IHJldHVybiBzaXplb2YoVHlwZSkgPD0g ODsgfQorICAgICAgICBzdHJ1Y3QgaXM2NEJpdFR5cGUgeworICAgICAgICAgICAgc3RhdGljIGNv bnN0ZXhwciBib29sIHZhbHVlID0gc2l6ZW9mKFR5cGUpIDw9IDg7CisgICAgICAgIH07CiAKICAg ICAgICAgdGVtcGxhdGU8PgotICAgICAgICBzdGF0aWMgY29uc3RleHByIGJvb2wgaXM2NEJpdFR5 cGU8dm9pZD4oKSB7IHJldHVybiB0cnVlOyB9CisgICAgICAgIHN0cnVjdCBpczY0Qml0VHlwZTx2 b2lkPiB7CisgICAgICAgICAgICBzdGF0aWMgY29uc3RleHByIGJvb2wgdmFsdWUgPSB0cnVlOwor ICAgICAgICB9OwogCiAgICAgICAgIHRlbXBsYXRlPHR5cGVuYW1lIE9wZXJhdGlvblR5cGUsIHR5 cGVuYW1lLi4uIEFyZ3M+CiAgICAgICAgIHN0ZDo6ZW5hYmxlX2lmX3Q8IXN0ZDo6aXNfc2FtZTx0 eXBlbmFtZSBGdW5jdGlvblRyYWl0czxPcGVyYXRpb25UeXBlPjo6UmVzdWx0VHlwZSwgU2xvd1Bh dGhSZXR1cm5UeXBlPjo6dmFsdWUsIE1hY3JvQXNzZW1ibGVyOjpDYWxsPgogICAgICAgICBjYWxs T3BlcmF0aW9uKE9wZXJhdGlvblR5cGUgb3BlcmF0aW9uLCBBcmdzLi4uIGFyZ3MpCiAgICAgICAg IHsKLSAgICAgICAgICAgIHN0YXRpY19hc3NlcnQoaXM2NEJpdFR5cGU8dHlwZW5hbWUgRnVuY3Rp b25UcmFpdHM8T3BlcmF0aW9uVHlwZT46OlJlc3VsdFR5cGU+KCksICJXaW42NCBjYW5ub3QgdXNl IHN0YW5kYXJkIGNhbGwgd2hlbiByZXR1cm4gdHlwZSBpcyBsYXJnZXIgdGhhbiA2NCBiaXRzLiIp OworICAgICAgICAgICAgc3RhdGljX2Fzc2VydChpczY0Qml0VHlwZTx0eXBlbmFtZSBGdW5jdGlv blRyYWl0czxPcGVyYXRpb25UeXBlPjo6UmVzdWx0VHlwZT46OnZhbHVlLCAiV2luNjQgY2Fubm90 IHVzZSBzdGFuZGFyZCBjYWxsIHdoZW4gcmV0dXJuIHR5cGUgaXMgbGFyZ2VyIHRoYW4gNjQgYml0 cy4iKTsKICAgICAgICAgICAgIHNldHVwQXJndW1lbnRzPE9wZXJhdGlvblR5cGU+KGFyZ3MuLi4p OwogICAgICAgICAgICAgcmV0dXJuIGFwcGVuZENhbGxXaXRoRXhjZXB0aW9uQ2hlY2sob3BlcmF0 aW9uKTsKICAgICAgICAgfQo=