Summary: | [WTF] Remove the unnecessary inner class DefaultHash<T>::Hash | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||||
Component: | Web Template Framework | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aboxhall, alecflett, apinheiro, bburg, beidson, benjamin, cdumez, cfleizach, cgarcia, changseok, cmarcelo, darin, dino, dmazzoni, esprehn+autocc, ews-watchlist, fmalita, glenn, graouts, gyuyoung.kim, hi, japhet, jcraig, jdiggs, jfernandez, joepeck, jsbell, kangil.han, keith_miller, kondapallykalyan, macpherson, mark.lam, menard, mmaxfield, msaboff, pdr, rego, saam, sabouhallawa, samuel_white, schenney, sergio, svillar, tzagallo, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Fujii Hironori
2020-07-15 17:50:20 PDT
Created attachment 404408 [details]
Patch
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`) Comment on attachment 404408 [details]
Patch
r- because the patch is causing lots of failures.
Created attachment 404413 [details]
Patch
Created attachment 404415 [details]
Patch
Created attachment 404422 [details]
Patch
Comment on attachment 404422 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404422&action=review > Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:532 > +template<JSC::PtrTag tag> struct DefaultHash<JSC::MacroAssemblerCodePtr<tag>> : JSC::MacroAssemblerCodePtrHash<tag> { }; Can we use "using" here instead of inheritance? I’d rather not have a separate DefaultHash that is not the same as MacroAssemblerCodePtrHash, in case that might result in a little code bloat. Same question about the many other places below where we define a new struct/class instead of doing "using". Looks like a good idea, that is working out well. Please consider my suggestion. Comment on attachment 404422 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404422&action=review >> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:532 >> +template<JSC::PtrTag tag> struct DefaultHash<JSC::MacroAssemblerCodePtr<tag>> : JSC::MacroAssemblerCodePtrHash<tag> { }; > > Can we use "using" here instead of inheritance? I’d rather not have a separate DefaultHash that is not the same as MacroAssemblerCodePtrHash, in case that might result in a little code bloat. > > Same question about the many other places below where we define a new struct/class instead of doing "using". I also had the same idea, but it doesn't work. It should be a template specialization of DefaultHash. Comment on attachment 404422 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404422&action=review >>> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:532 >>> +template<JSC::PtrTag tag> struct DefaultHash<JSC::MacroAssemblerCodePtr<tag>> : JSC::MacroAssemblerCodePtrHash<tag> { }; >> >> Can we use "using" here instead of inheritance? I’d rather not have a separate DefaultHash that is not the same as MacroAssemblerCodePtrHash, in case that might result in a little code bloat. >> >> Same question about the many other places below where we define a new struct/class instead of doing "using". > > I also had the same idea, but it doesn't work. > It should be a template specialization of DefaultHash. This, then, points to the benefit of the old scheme. The DefaultHash expression evaluated to a type, and it could also be specified directly, and in both cases it’s the same type. With this new scheme, DefaultHash itself is a separate type. So that could lead to a bit of extra code bloat. Maybe in practice that’s not a problem. I like the benefits of this for reducing the need for class definitions, and that it’s simpler. I’d like to hear from C++ template meta programming efforts. Comment on attachment 404422 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404422&action=review >>>> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:532 >>>> +template<JSC::PtrTag tag> struct DefaultHash<JSC::MacroAssemblerCodePtr<tag>> : JSC::MacroAssemblerCodePtrHash<tag> { }; >>> >>> Can we use "using" here instead of inheritance? I’d rather not have a separate DefaultHash that is not the same as MacroAssemblerCodePtrHash, in case that might result in a little code bloat. >>> >>> Same question about the many other places below where we define a new struct/class instead of doing "using". >> >> I also had the same idea, but it doesn't work. >> It should be a template specialization of DefaultHash. > > This, then, points to the benefit of the old scheme. The DefaultHash expression evaluated to a type, and it could also be specified directly, and in both cases it’s the same type. > > With this new scheme, DefaultHash itself is a separate type. So that could lead to a bit of extra code bloat. Maybe in practice that’s not a problem. > > I like the benefits of this for reducing the need for class definitions, and that it’s simpler. > > I’d like to hear from C++ template meta programming efforts. experts, not efforts Comment on attachment 404422 [details] Patch Clearing flags on attachment: 404422 Committed r264488: <https://trac.webkit.org/changeset/264488> All reviewed patches have been landed. Closing bug. (In reply to Fujii Hironori from comment #13) > Comment on attachment 404422 [details] > Patch > > Clearing flags on attachment: 404422 > > Committed r264488: <https://trac.webkit.org/changeset/264488> I thought Darin said that we should not go with this approach of subclassing. Am I missing something? Darin didn't object to the subclassing. (In reply to Darin Adler from comment #11) > I like the benefits of this for reducing the need for class definitions, and > that it’s simpler. (In reply to Mark Lam from comment #16) > (In reply to Fujii Hironori from comment #13) > I thought Darin said that we should not go with this approach of > subclassing. Am I missing something? I suggested another approach, and he explained that he tried it and it did not work. |