1449985 - Replace IsDelegate with IsDelegateOfObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[André Bargull [:anba]]

We have |js::IsDelegate(JSContext*, HandleObject, const js::Value&, bool*)| and |js::IsDelegateOfObject(JSContext*, HandleObject, JSObject*, bool*)| [1], but |IsDelegate(...)| is always called with an Object value [2,3], which means both callers could directly call |IsDelegateOfObject(...)| and |IsDelegate(...)| can be removed.

As a follow-up we could then consider if makes sense to rename IsDelegateOfObject to IsDelegate.


[1] https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/js/src/vm/JSObject.cpp#3180,3190
[2] https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/js/src/builtin/Object.cpp#942
[3] https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/js/src/vm/JSFunction.cpp#775

Comment 1

[Jan de Mooij [:jandem]]

(In reply to André Bargull [:anba] from comment #0)
> As a follow-up we could then consider if makes sense to rename
> IsDelegateOfObject to IsDelegate.

I'd suggest IsPrototypeOf or HasOnProtoChain...

Comment 2

[Ashley Hauck [:khyperia]]

Attachment: Remove js::IsDelegate, rename IsDelegateOfObject to IsPrototypeOf.

Comment 3

[Ashley Hauck [:khyperia]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a17559f22e1b07d6314c5142cc1256939619fa3

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 9003556 [details] [diff] [review]
Remove js::IsDelegate, rename IsDelegateOfObject to IsPrototypeOf.

Review of attachment 9003556 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

::: js/src/jit/CodeGenerator.cpp
@@ +12197,2 @@
>  static bool
> +IsPrototype(JSContext* cx, HandleObject protoObj, HandleObject obj, bool* res)

Pre-existing: this wrapper function was necessary at some point for a different reason, but it no longer is. Mind removing it and using IsPrototypeOf directly in the code below? You'll have to change IsPrototypeFn to have a matching signature, JSObject* instead of HandleObject, to get it to compile. Thanks!

Comment 5

[Ashley Hauck [:khyperia]]

Attachment: Remove js::IsDelegate, rename IsDelegateOfObject to IsPrototypeOf.

Comment 6

[Ashley Hauck [:khyperia]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> Pre-existing: this wrapper function was necessary at some point for a
> different reason, but it no longer is. Mind removing it and using
> IsPrototypeOf directly in the code below? You'll have to change
> IsPrototypeFn to have a matching signature, JSObject* instead of
> HandleObject, to get it to compile. Thanks!

I'm confused on why the callsite of that function pointer didn't have to get updated to use JSObject instead of HandleObject... from your comment, doing nothing seems like the right thing to do, but I want to make sure.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 9003810 [details] [diff] [review]
Remove js::IsDelegate, rename IsDelegateOfObject to IsPrototypeOf.

Review of attachment 9003810 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Ashley Hauck [:khyperia] from comment #6)
> I'm confused on why the callsite of that function pointer didn't have to get
> updated to use JSObject instead of HandleObject... from your comment, doing
> nothing seems like the right thing to do, but I want to make sure.

We have the JSObject* in a register (objReg) here:

https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/js/src/jit/CodeGenerator.cpp#12108-12110

The static VMFunction definition is used to create a trampoline for this particular C++ function. The trampoline uses the signature to decide how to pass the argument. For HandleObject it knows to pass a pointer-to-JSObject* instead of the JSObject*:

https://searchfox.org/mozilla-central/rev/e126996d9b0a3d7653de205898517f4f5b632e7f/js/src/jit/VMFunctions.h#373-375

So we generate slightly different code for JSObject* vs HandleObject but that's handled automatically.

Comment 9

[Ashley Hauck [:khyperia]]

(In reply to Jan de Mooij [:jandem] from comment #8)
> So we generate slightly different code for JSObject* vs HandleObject but
> that's handled automatically.

Thanks for the explanation!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba637657bbf8
Remove js::IsDelegate, rename IsDelegateOfObject to IsPrototypeOf. r=jandem

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/ba637657bbf8