Bug 78071 – Remove implicit copy constructor usage in HashMaps with OwnPtr

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

[Adrienne Walker]

Remove implicit copy constructor usage in HashMaps with OwnPtr

[Adrienne Walker]

Created attachment 125987 [details]
Patch

[Adrienne Walker]

My kingdom for an auto keyword.

I kept this patch separate from the one in 74154, since that's probably less interesting to non-Chromium folks.

[Philippe Normand]

Comment on attachment 125987 [details]
Patch

Attachment 125987 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11459247

[Early Warning System Bot]

Comment on attachment 125987 [details]
Patch

Attachment 125987 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11462235

[Gyuyoung Kim]

Comment on attachment 125987 [details]
Patch

Attachment 125987 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11453233

[Eric Seidel (no email)]

Comment on attachment 125987 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125987&action=review

> Source/JavaScriptCore/wtf/HashTraits.h:118
>          static std::nullptr_t emptyValue() { return nullptr; }

Should this use EmptyType?  (Are there other places which should?)

[Adrienne Walker]

Created attachment 126408 [details]
EWS build fixes

[Adrienne Walker]

(In reply to comment #6)
> (From update of attachment 125987 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125987&action=review
> 
> > Source/JavaScriptCore/wtf/HashTraits.h:118
> >          static std::nullptr_t emptyValue() { return nullptr; }
> 
> Should this use EmptyType?  (Are there other places which should?)

For consistency, I updated that one, but I think all the other cases should still be correct.  At the end of the day, emptyValue() will always get converted to TraitType.  The only difference EmptyType makes is what conversions that value goes through to get there.

This is really a case for C++11's decltype, where EmptyType wouldn't be needed at all, but what can you do...

[Darin Adler]

Comment on attachment 126408 [details]
EWS build fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=126408&action=review

Looks good.

> Source/JavaScriptCore/runtime/StructureTransitionTable.h:58
> +    typedef WTF::PairHashTraits<WTF::HashTraits<RefPtr<StringImpl> >, WTF::GenericHashTraits<unsigned> > HashTraits;

No need for the WTF:: prefix on PairHashTraits and HashTraits here. Not sure why that was done in the old code.

The second half can be HashTraits<unsigned>, not GenericHashTraits<unsigned>, since these are identical types.

Once we do those two things we get this:

    typedef PairHashTraits<HashTraits<RefPtr<StringImpl> >, HashTraits<unsigned> > HashTraits.

And if we look at that, it becomes clear that these are the default hash traits you’d get for this type; there is no specialization here! Because of that, there is need for this HashTraits typedef at all. We can just leave out the HashTraits argument in the typedef for the TransitionMap type and the default template argument will do the right thing.

> Source/JavaScriptCore/wtf/HashTraits.h:60
> +        typedef T EmptyType;

I’d be tempted to use a longer name, EmptyValueType, here.

[Darin Adler]

Comment on attachment 125987 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125987&action=review

>>> Source/JavaScriptCore/wtf/HashTraits.h:118
>>>          static std::nullptr_t emptyValue() { return nullptr; }
>> 
>> Should this use EmptyType?  (Are there other places which should?)
> 
> For consistency, I updated that one, but I think all the other cases should still be correct.  At the end of the day, emptyValue() will always get converted to TraitType.  The only difference EmptyType makes is what conversions that value goes through to get there.
> 
> This is really a case for C++11's decltype, where EmptyType wouldn't be needed at all, but what can you do...

When there are two different ways to state the type, I don’t think there’s a strong reason to use the name EmptyType over the other name for the same type, std::nullptr_t. Either is fine.

You’ll note in other similar code that we tend to use the named type only when the type is complicated. So we use PeekType for return values below but we do not use PassOutType.

Wholeheartedly agreed that this class will become much more elegant once we can use C++11 features, but having that as a requirement for compiling TOT is probably a few years ahead for the WebKit project.

[Adrienne Walker]

Created attachment 126422 [details]
Address review comments

[Adrienne Walker]

(In reply to comment #9)

> And if we look at that, it becomes clear that these are the default hash traits you’d get for this type; there is no specialization here! Because of that, there is need for this HashTraits typedef at all.

> We can just leave out the HashTraits argument in the typedef for the TransitionMap type and the default template argument will do the right thing.

Oh, excellent thought.  Done.

> > Source/JavaScriptCore/wtf/HashTraits.h:60
> > +        typedef T EmptyType;
> 
> I’d be tempted to use a longer name, EmptyValueType, here.

Done.

[WebKit Review Bot]

Comment on attachment 126422 [details]
Address review comments

Clearing flags on attachment: 126422

Committed r107421: <http://trac.webkit.org/changeset/107421>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.