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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 9 17:13:21 PDT 2011

Geoffrey Garen <ggaren at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 67727: Remove toBoolean from JSCell

Attachment 106920: Patch

------- 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
> +	   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.

More information about the webkit-reviews mailing list