Closed Bug 1140586 Opened 7 years ago Closed 7 years ago

Split up js::NewFunction into scripted and native versions

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(5 files)

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.
Attachment #8575012 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8575013 - Flags: review?(jwalden+bmo) → review+
Attachment #8575014 - Flags: review?(jwalden+bmo) → review+
Attachment #8575015 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8575016 - Flags: review?(jwalden+bmo) → review+
> 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...
You need to log in before you can comment on or make changes to this bug.