[JSC] isCallable is redundant with isFunction
Created attachment 397603 [details] Patch
Created attachment 397606 [details] Patch
Comment on attachment 397606 [details] Patch r=me
Committed r260722: <https://trac.webkit.org/changeset/260722> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397606 [details].
<rdar://problem/62380487>
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.
(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.
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.
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
(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”
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.
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.