Bug 67875 – Remove toPrimitive from JSCell

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

[Mark Hahnenberg]

We should remove JSCell::toPrimitive in favor of JSValue::toPrimitive.  The JSValue version in non-virtual, and we're trying to remove all virtual methods from JSCell.

[Mark Hahnenberg]

Created attachment 106945 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 106945 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        all of the implicit functionality provided by the virutal toPrimitive method 

sp

> Source/JavaScriptCore/runtime/JSObject.h:911
> +        return *const_cast<JSValue*>(this);

no const_cast, please

> Source/JavaScriptCore/runtime/JSObject.h:912
> +    if (isCell())

Test isCell first for efficiency. Create separate toPrimitives for JSCell* and JSObject*.

[Mark Hahnenberg]

Created attachment 107708 [details]
Patch

[WebKit Review Bot]

Comment on attachment 107708 [details]
Patch

Attachment 107708 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9729359

[Geoffrey Garen]

Comment on attachment 107708 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSCell.cpp:124
> +    if (isString())
> +        return ((JSString*)this)->toPrimitive(exec, preferredType);
> +    return ((JSObject*)this)->toPrimitive(exec, preferredType);

Use C++ static_cast, please. It's more explicit, and provides a little more type safety than a C-style cast.

Are the bot complaints a real issue?

[Mark Hahnenberg]

Created attachment 107892 [details]
Patch

[Geoffrey Garen]

Comment on attachment 107892 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 107892 [details]
Patch

Clearing flags on attachment: 107892

Committed r95466: <http://trac.webkit.org/changeset/95466>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Why did this patch move JSValue::toPrimitive from JSCell.h to JSString.h? That change makes no sense to me!

[Mark Hahnenberg]

(In reply to comment #10)
> Why did this patch move JSValue::toPrimitive from JSCell.h to JSString.h? That change makes no sense to me!

Good question.  This is the corrected (for compilation) version of an incorrect (for style) version.  Rolling it out to resubmit the corrected (for both) version.

[Mark Hahnenberg]

Created attachment 107922 [details]
Patch

[WebKit Review Bot]

Comment on attachment 107922 [details]
Patch

Clearing flags on attachment: 107922

Committed r95516: <http://trac.webkit.org/changeset/95516>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.