Bug 122802 – Don't generate a wasteful isObservable check in isReachableFromOpaqueRoots

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Like here:

bool JSTestObjOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor)
{
    JSTestObj* jsTestObj = jsCast<JSTestObj*>(handle.get().asCell());
    if (!isObservable(jsTestObj))
        return false;
    UNUSED_PARAM(visitor);
    return false;
}

[Alexey Proskuryakov]

Created attachment 214214 [details]
proposed patch

The added UNUSED_PARAM is not always accurate, but neither was an existing one.

[Mark Hahnenberg]

Comment on attachment 214214 [details]
proposed patch

r=me

[Alexey Proskuryakov]

Created attachment 214215 [details]
a little better

Actually, can do a little better.

[Mark Hahnenberg]

Comment on attachment 214215 [details]
a little better

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

r=me

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2575
> +            push(@implContent, "    JS${interfaceName}* js${interfaceName} = jsCast<JS${interfaceName}*>(handle.get().asCell());\n");
> +            $emittedJSCast = 1;

Might want to check if (!$emittedJSCast) for future people to pick up on the pattern if they add something. Up to you though.

[Alexey Proskuryakov]

Comment on attachment 214215 [details]
a little better

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2575
>> +            $emittedJSCast = 1;
> 
> Might want to check if (!$emittedJSCast) for future people to pick up on the pattern if they add something. Up to you though.

Yeah, I also had this thought cross my mind... I think that adding an unnecessary check would be confusing, and that does not overweigh the benefit of easier copy/pasting.

[WebKit Commit Bot]

Comment on attachment 214215 [details]
a little better

Clearing flags on attachment: 214215

Committed r157438: <http://trac.webkit.org/changeset/157438>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.