Bug 78071

Summary: Remove implicit copy constructor usage in HashMaps with OwnPtr
Product: WebKit Reporter: Adrienne Walker <enne>
Component: Web Template FrameworkAssignee: Adrienne Walker <enne>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, darin, enne, gns, jamesr, pnormand, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 74154    
Attachments:
Description Flags
Patch
none
EWS build fixes
none
Address review comments none

Description Adrienne Walker 2012-02-07 19:27:52 PST
Remove implicit copy constructor usage in HashMaps with OwnPtr
Comment 1 Adrienne Walker 2012-02-07 19:33:01 PST
Created attachment 125987 [details]
Patch
Comment 2 Adrienne Walker 2012-02-07 19:36:50 PST
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.
Comment 3 Philippe Normand 2012-02-07 19:39:08 PST
Comment on attachment 125987 [details]
Patch

Attachment 125987 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11459247
Comment 4 Early Warning System Bot 2012-02-07 20:31:42 PST
Comment on attachment 125987 [details]
Patch

Attachment 125987 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11462235
Comment 5 Gyuyoung Kim 2012-02-07 20:52:16 PST
Comment on attachment 125987 [details]
Patch

Attachment 125987 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11453233
Comment 6 Eric Seidel (no email) 2012-02-08 15:54:17 PST
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?)
Comment 7 Adrienne Walker 2012-02-09 17:09:43 PST
Created attachment 126408 [details]
EWS build fixes
Comment 8 Adrienne Walker 2012-02-09 17:24:48 PST
(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...
Comment 9 Darin Adler 2012-02-09 17:26:33 PST
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.
Comment 10 Darin Adler 2012-02-09 17:29:03 PST
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.
Comment 11 Adrienne Walker 2012-02-09 18:17:32 PST
Created attachment 126422 [details]
Address review comments
Comment 12 Adrienne Walker 2012-02-09 18:18:52 PST
(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.
Comment 13 WebKit Review Bot 2012-02-10 11:36:55 PST
Comment on attachment 126422 [details]
Address review comments

Clearing flags on attachment: 126422

Committed r107421: <http://trac.webkit.org/changeset/107421>
Comment 14 WebKit Review Bot 2012-02-10 11:37:01 PST
All reviewed patches have been landed.  Closing bug.