[webkit-reviews] review denied: [Bug 67727] Remove toBoolean from JSCell : [Attachment 106920] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 67727: Remove toBoolean from JSCell
https://bugs.webkit.org/show_bug.cgi?id=67727

Attachment 106920: Patch
https://bugs.webkit.org/attachment.cgi?id=106920&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=106920&action=review


If you find any other implementations of toBoolean, I think you should remove
those too. I believe there is at least one more you haven't removed yet.

> Source/JavaScriptCore/runtime/JSString.h:696
> +	   if (isString())
> +	       return static_cast<JSString*>(asCell())->toBoolean(exec);
> +	   if (isCell())
> +	       return
!asCell()->structure()->typeInfo().masqueradesAsUndefined();
> +	   return isTrue(); // false, null, and undefined all convert to false.


You could remove one branch by reordering here:

if (!isCell())
    return isTrue();
if (asCell()->isString())
    return static_cast<JSString*>(asCell())->isEmpty();
return asCell()->structure()->typeInfo().masqueradesAsUndefined();

> Source/JavaScriptCore/runtime/StringObjectThatMasqueradesAsUndefined.h:55
> -	   virtual bool toBoolean(ExecState*) const { return false; }
> +	   bool toBoolean(ExecState*) const { return false; }

This is no good -- JSValue already does the right thing and overriding
non-virtual functions is not a good idea.