Bug 211037 – [JSC] isCallable is redundant with isFunction

• Component: New Bugs
• Status: RESOLVED FIXED

[Ross Kirsling]

[JSC] isCallable is redundant with isFunction

[Ross Kirsling]

Created attachment 397603 [details]
Patch

[Ross Kirsling]

Created attachment 397606 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 397606 [details]
Patch

r=me

[EWS]

Committed r260722: <https://trac.webkit.org/changeset/260722>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397606 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62380487>

[Darin Adler]

Two other thoughts:

1) Is there something analogous we should be doing with getConstructData?

2) Seems that getCallData could be changed to have a return value, instead of a return value plus two out arguments, and be renamed to callData. Could be more elegant and similar in performance, maybe better.

[Alexey Shvayka]

(In reply to EWS from comment #4)
> Committed r260722: <https://trac.webkit.org/changeset/260722>

I really like this change, especially the usage of isFunction's fast path. Great job, Ross!
We might also want to use isFunction() in JSObjectIsFunction() of JSObjectRef.cpp.

(In reply to Darin Adler from comment #6)
> 2) Seems that getCallData could be changed to have a return value, instead
> of a return value plus two out arguments, and be renamed to callData. Could
> be more elegant and similar in performance, maybe better.

getCallData() seems to be nicely aligned with getConstructData(): they both return a value + one out argument.
However, isConstructor() has an overload which returns a value + two out arguments, with only one call site.
It would be nice to use getConstructData() instead.

[Ross Kirsling]

I hadn't looked into getConstructData but I'll do so in a follow-up bug!

(In reply to Alexey Shvayka from comment #7)
> We might also want to use isFunction() in JSObjectIsFunction() of
> JSObjectRef.cpp.

Oops, yeah, that was an oversight on my part! Will add to the next patch.

[Saam Barati]

Comment on attachment 397606 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        Where CallData is needed, getCallData should be used; where CallData is not needed, isFunction should be used.

I’m not a fan of us having the “isFunction” name. I would’ve opted for isCallable everywhere. isFunction js ambiguous when I read it. E.g, Is it asking if it’s a JSFunction?

All JSFunctions are callable, not all callable objects are JSFunctions

[Saam Barati]

(In reply to Saam Barati from comment #9)
> Comment on attachment 397606 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397606&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        Where CallData is needed, getCallData should be used; where CallData is not needed, isFunction should be used.
> 
> I’m not a fan of us having the “isFunction” name. I would’ve opted for
> isCallable everywhere. isFunction js ambiguous when I read it. E.g, Is it
> asking if it’s a JSFunction?
> 
> All JSFunctions are callable, not all callable objects are JSFunctions

Oops, I meant “is ambiguous”, not “js ambiguous”

[Darin Adler]

I suggest using do-webcore-rename to rename isFunction to isCallable. There’s very little else besides the isFunction in JavaScriptCore to collide so it should be largely mechanical.

[Ross Kirsling]

Yeah, I agree with that renaming direction. On the plus side, it'll be easier now that everything's clear-cut. Will open a new ticket shortly.