Bug 163490 – Build fix for HasOwnPropertyCache::Entry caused slowdown by introducing a constructor that doesn't use move semantics for the RefPtr<UniquedStringImpl> parameter

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 291843 [details]
patch

[Darin Adler]

Comment on attachment 291843 [details]
patch

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

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:48
> +        Entry(RefPtr<UniquedStringImpl>&& impl, StructureID structureID, bool result)
> +            : impl(WTFMove(impl))

Looks good.

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:124
> -            bitwise_cast<Entry*>(this)[index] = Entry{ RefPtr<UniquedStringImpl>(impl), id, result };
> +            bitwise_cast<Entry*>(this)[index] = Entry(RefPtr<UniquedStringImpl>(impl), id, result);

I suggest not making this change.

Using the function-call-style () syntax instead of the initialization-style {} syntax allows some kinds of implicit type conversions and it’s nice to not allow them. Generally, I think the { } syntax should be preferred in new code. In addition, Anders Carlsson convinced me that we should have a space after the type name, but I won’t try to convince you of that right now.

[Saam Barati]

Created attachment 291847 [details]
patch for landing

[Saam Barati]

Comment on attachment 291843 [details]
patch

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

>> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:124
>> +            bitwise_cast<Entry*>(this)[index] = Entry(RefPtr<UniquedStringImpl>(impl), id, result);
> 
> I suggest not making this change.
> 
> Using the function-call-style () syntax instead of the initialization-style {} syntax allows some kinds of implicit type conversions and it’s nice to not allow them. Generally, I think the { } syntax should be preferred in new code. In addition, Anders Carlsson convinced me that we should have a space after the type name, but I won’t try to convince you of that right now.

Ok, I've changed it back, and with the space between the type name and the "{". I actually prefer the space, however, I've never been aware of any style rules for WebKit itself. If we haven't decided on an official style, we probably should, and put it in the style guide.

I like the property of not allowing type conversions and allowing the caller to control that instead of the callee.

[WebKit Commit Bot]

Comment on attachment 291847 [details]
patch for landing

Clearing flags on attachment: 291847

Committed r207425: <http://trac.webkit.org/changeset/207425>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.