Bug 214389 – [WTF] Remove the unnecessary inner class DefaultHash<T>::Hash

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Fujii Hironori]

[WTF] Remove the unnecessary inner class DefaultHash<T>::Hash 

This is a problem for forward declaration (Bug 214320 Comment 5).

[Fujii Hironori]

Created attachment 404408 [details]
Patch

[EWS Watchlist]

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`)

[Mark Lam]

Comment on attachment 404408 [details]
Patch

r- because the patch is causing lots of failures.

[Fujii Hironori]

Created attachment 404413 [details]
Patch

[Fujii Hironori]

DefaultHash<T>::Hash was added by r12329.

[Fujii Hironori]

Created attachment 404415 [details]
Patch

[Fujii Hironori]

Created attachment 404422 [details]
Patch

[Darin Adler]

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

[Darin Adler]

Looks like a good idea, that is working out well. Please consider my suggestion.

[Fujii Hironori]

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.

[Darin Adler]

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.

[Darin Adler]

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

[Fujii Hironori]

Comment on attachment 404422 [details]
Patch

Clearing flags on attachment: 404422

Committed r264488: <https://trac.webkit.org/changeset/264488>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/65698688>

[Mark Lam]

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

[Fujii Hironori]

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.

[Darin Adler]

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