1140586 - Split up js::NewFunction into scripted and native versions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The scripted version would not take a native but would take a parent, for now.  The native version would take no funobj and no parent.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add some asserts about us not caring too much about the parents of native functions

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop passing non-null funobjArg to js::NewFunction and js::NewFunctionWithProto

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Drop the funobjArg of js::NewFunction and js::NewFunctionWithProto

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Stop passing non-null parents to js::NewFunctionWithProto when creating a native function via direct calls to that API

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Split up js::NewFunction into several different APIs that are more clear in terms of what they do and don't need parents as much

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8575013 [details] [diff] [review]
part 2.  Stop passing non-null funobjArg to js::NewFunction and js::NewFunctionWithProto

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

::: js/src/jsfun.cpp
@@ +777,5 @@
>  static JSObject *
>  CreateFunctionConstructor(JSContext *cx, JSProtoKey key)
>  {
>      Rooted<GlobalObject*> self(cx, cx->global());
> +    RootedObject functionProto(cx, &cx->global()->getPrototype(JSProto_Function).toObject());

Mild preference to s/self/global/ and use that here if you're changing stuff in this area.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8575016 [details] [diff] [review]
part 5.  Split up js::NewFunction into several different APIs that are more clear in terms of what they do and don't need parents as much

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

::: js/src/jsapi.cpp
@@ +3148,5 @@
>      }
>  
> +    return (flags & JSFUN_CONSTRUCTOR) ?
> +        NewNativeConstructor(cx, native, nargs, atom) :
> +        NewNativeFunction(cx, native, nargs, atom);

return (flags & JSFUN_CONSTRUCTOR)
       ? NewNativeConstructor(...)
       : NewNativeFunction(...);

@@ +3164,5 @@
>  
>      RootedAtom name(cx, JSID_TO_ATOM(id));
> +    return (flags & JSFUN_CONSTRUCTOR) ?
> +        NewNativeConstructor(cx, native, nargs, name) :
> +        NewNativeFunction(cx, native, nargs, name);

Same styling.

::: js/src/jsfun.cpp
@@ +1706,5 @@
>      if (!fun->initBoundFunction(cx, target, thisArg, boundArgs, argslen))
>          return nullptr;
>  
> +    // Steps 9-10. Set length again, because NewNativeFunction/NewNativeConstructor
> +    // sometimes truncates.

/me eyes that comment beadily

@@ +1997,5 @@
>  
>  JSFunction *
> +js::NewNativeFunction(ExclusiveContext *cx, Native native, unsigned nargs, HandleAtom atom,
> +                      gc::AllocKind allocKind /* = JSFunction::FinalizeKind */,
> +                      NewObjectKind newKind /* = GenericObject */)

If newKind is unused here, you should remove it from the signature and all callers, right?

@@ +2009,5 @@
> +                         gc::AllocKind allocKind /* = JSFunction::FinalizeKind */,
> +                         NewObjectKind newKind /* = GenericObject */,
> +                         JSFunction::Flags flags /* = JSFunction::NATIVE_CTOR */)
> +{
> +    MOZ_ASSERT(flags & JSFunction::NATIVE_CTOR);

I might (separate patch) make this separateFlags and not require NATIVE_CTOR be passed by callers (indeed, *require* that it not be passed).  Separate patch.

::: js/src/jsfun.h
@@ +514,5 @@
> +                     gc::AllocKind allocKind = JSFunction::FinalizeKind,
> +                     NewObjectKind newKind = GenericObject,
> +                     JSFunction::Flags flags = JSFunction::NATIVE_CTOR);
> +
> +// Allocate a new scripted function backed by a JSNative.

s/ backed by a JSNative//

::: js/src/vm/SharedArrayObject.cpp
@@ +345,5 @@
>  
>      RootedId byteLengthId(cx, NameToId(cx->names().byteLength));
>      unsigned attrs = JSPROP_SHARED | JSPROP_GETTER | JSPROP_PERMANENT;
> +    JSObject *getter =
> +        NewNativeFunction(cx, SharedArrayBufferObject::byteLengthGetter, 0, NullPtr());

I'm not a huge fan of these NullPtr() everywhere with no obvious meaning.  :-\  Maybe I'll get used to it when there's no parent and it can only be one thing.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Mild preference to s/self/global/ and use that here if you're changing stuff in this area.

Done.

> /me eyes that comment beadily

I just moved it!  But yes, it sounds bogosityfull.

> If newKind is unused here, you should remove it from the signature and all callers,
> right?

Ouch.  Good catch.  It should be passed to NewFunctionWithProto.  

It _might_ be unused if it's never passed to js::DefineFunction when passing a Native, but I'd rather not assume that for the moment.

> I might (separate patch) make this separateFlags

I'm not sure it's worth the bother.  The only callers who pass something weird here are asm.js, basically.

> s/ backed by a JSNative//

Er, yes.  ;)

> Maybe I'll get used to it when there's no parent and it can only be one thing.

Yeah...

Comment 9

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b60ccf33795d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d7fdba0593
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8eb9a843bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/56e47c184dcc
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd142e2ac19c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b60ccf33795d
https://hg.mozilla.org/mozilla-central/rev/e2d7fdba0593
https://hg.mozilla.org/mozilla-central/rev/2d8eb9a843bf
https://hg.mozilla.org/mozilla-central/rev/56e47c184dcc
https://hg.mozilla.org/mozilla-central/rev/bd142e2ac19c