RESOLVED FIXED 67875
Remove toPrimitive from JSCell
https://bugs.webkit.org/show_bug.cgi?id=67875
Summary Remove toPrimitive from JSCell
Mark Hahnenberg
Reported 2011-09-09 16:32:35 PDT
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.
Attachments
Patch (10.58 KB, patch)
2011-09-09 17:23 PDT, Mark Hahnenberg
no flags
Patch (6.46 KB, patch)
2011-09-16 12:56 PDT, Mark Hahnenberg
no flags
Patch (8.60 KB, patch)
2011-09-19 11:28 PDT, Mark Hahnenberg
no flags
Patch (7.60 KB, patch)
2011-09-19 15:01 PDT, Mark Hahnenberg
no flags
Mark Hahnenberg
Comment 1 2011-09-09 17:23:39 PDT
Mark Hahnenberg
Comment 2 2011-09-13 15:05:25 PDT
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
Comment 3 2011-09-16 12:56:25 PDT
WebKit Review Bot
Comment 4 2011-09-17 02:20:10 PDT
Geoffrey Garen
Comment 5 2011-09-18 15:44:50 PDT
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
Comment 6 2011-09-19 11:28:39 PDT
Geoffrey Garen
Comment 7 2011-09-19 12:06:35 PDT
Comment on attachment 107892 [details] Patch r=me
WebKit Review Bot
Comment 8 2011-09-19 13:31:59 PDT
Comment on attachment 107892 [details] Patch Clearing flags on attachment: 107892 Committed r95466: <http://trac.webkit.org/changeset/95466>
WebKit Review Bot
Comment 9 2011-09-19 13:32:04 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 10 2011-09-19 14:08:38 PDT
Why did this patch move JSValue::toPrimitive from JSCell.h to JSString.h? That change makes no sense to me!
Mark Hahnenberg
Comment 11 2011-09-19 14:19:45 PDT
(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
Comment 12 2011-09-19 15:01:42 PDT
WebKit Review Bot
Comment 13 2011-09-19 21:32:18 PDT
Comment on attachment 107922 [details] Patch Clearing flags on attachment: 107922 Committed r95516: <http://trac.webkit.org/changeset/95516>
WebKit Review Bot
Comment 14 2011-09-19 21:32:23 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.