[WTF] Remove the unnecessary inner class DefaultHash<T>::Hash This is a problem for forward declaration (Bug 214320 Comment 5).
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
DefaultHash<T>::Hash was added by r12329.
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.
<rdar://problem/65698688>
(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.