Closed Bug 992958 Opened 9 years ago Closed 8 years ago

Give [Object] and [Function] a ClassSpec

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(11 files, 1 obsolete file)

1.46 KB, patch
luke
: review+
Details | Diff | Splinter Review
5.70 KB, patch
luke
: review+
Details | Diff | Splinter Review
10.83 KB, patch
luke
: review+
Details | Diff | Splinter Review
3.43 KB, patch
luke
: review+
Details | Diff | Splinter Review
5.44 KB, patch
luke
: review+
Details | Diff | Splinter Review
8.49 KB, patch
luke
: review+
Details | Diff | Splinter Review
7.98 KB, patch
luke
: review+
Details | Diff | Splinter Review
9.81 KB, patch
luke
: review+
Details | Diff | Splinter Review
12.38 KB, patch
luke
: review+
Details | Diff | Splinter Review
20.14 KB, patch
luke
: review+
Details | Diff | Splinter Review
10.93 KB, patch
luke
: review+
Details | Diff | Splinter Review
We need the standard class information here to be declarative so that we can hook into it for Xrays.
(Object and Function are pretty inextricable)
Summary: Give [Object] a ClassSpec → Give [Object] and [Function] a ClassSpec
Try push: https://tbpl.mozilla.org/?tree=Try&rev=d54e58be195d

This is green modulo a couple of failures that I've fixed. Uploading patches and flagging for review
The need for this is long-gone, I believe.
Attachment #8423556 - Flags: review?(luke)
We don't need the nativeCall bits anymore.
Attachment #8423557 - Flags: review?(luke)
When we decouple function and object, we'll need to rely on these slots being
set up as soon as the relevant objects are created.
Attachment #8423560 - Flags: review?(luke)
We try to keep the diff small for now, and reformat in the next patch.
Attachment #8423561 - Flags: review?(luke)
This is a pure move, aside from adding a js:: namespace to a few NullPtrs.
Attachment #8423565 - Flags: review?(luke)
Comment on attachment 8423556 [details] [diff] [review]
Part 2 - Stop going through the __proto__ getter in getPrototypeOf. v1

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

::: js/src/vm/GlobalObject.cpp
@@ -365,5 @@
>      {
>          return nullptr;
>      }
>  #endif /* JS_HAS_OBJ_PROTO_PROP */
> -    self->setProtoGetter(getter);

If you care about that, the definition of getter can now be inside the JS_HAS_OBJ_PROTO_PROP block.
Attachment #8423558 - Attachment is obsolete: true
Attachment #8423558 - Flags: review?(luke)
Attachment #8423951 - Flags: review?(luke)
Attachment #8423555 - Flags: review?(luke) → review+
Comment on attachment 8423556 [details] [diff] [review]
Part 2 - Stop going through the __proto__ getter in getPrototypeOf. v1

Reassigning to Waldo; otherwise I'd just be rubberstamping.
Attachment #8423556 - Flags: review?(luke) → review?(jwalden+bmo)
Attachment #8423557 - Flags: review?(luke) → review?(jwalden+bmo)
Comment on attachment 8423559 [details] [diff] [review]
Part 5 - Rejigger the ordering of resolveConstructor to make it work for Object/Function. v1

Yeah... actually all of these patches exceed my understanding.  Waldo did the last major refactoring of this area.
Attachment #8423559 - Flags: review?(luke) → review?(jwalden+bmo)
Attachment #8423560 - Flags: review?(luke) → review?(jwalden+bmo)
Attachment #8423561 - Flags: review?(luke) → review?(jwalden+bmo)
Attachment #8423562 - Flags: review?(luke) → review?(jwalden+bmo)
Comment on attachment 8423563 [details] [diff] [review]
Part 9 - Reindenting and trivial cleanup. v1

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

I got this one.

::: js/src/vm/GlobalObject.cpp
@@ +141,5 @@
> +    JSObject *functionProto_ = NewObjectWithGivenProto(cx, &JSFunction::class_,
> +                                                       objectProto, self, SingletonObject);
> +    if (!functionProto_)
> +        return nullptr;
> +    RootedFunction functionProto(cx, &functionProto_->as<JSFunction>());

Preexisting in this area of the code: can you add \n after branches before the next line of code?  That's the usual SM style.

@@ +218,5 @@
> +    if (!ctor)
> +        return nullptr;
> +    RootedAtom objectAtom(cx, cx->names().Object);
> +    return NewFunction(cx, ctor, obj_construct, 1, JSFunction::NATIVE_CTOR, self,
> +                       objectAtom);

Preexisting, but I think you can pass cx->names().Object directly to NewFunction w/o the Rooted.  Same thing for functionAtom below.
Attachment #8423563 - Flags: review?(luke) → review+
Attachment #8423564 - Flags: review?(luke) → review?(jwalden+bmo)
Attachment #8423565 - Flags: review?(luke) → review?(jwalden+bmo)
Attachment #8423951 - Flags: review?(luke) → review?(jwalden+bmo)
Attachment #8423556 - Flags: review?(jwalden+bmo) → review+
Attachment #8423557 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8423559 [details] [diff] [review]
Part 5 - Rejigger the ordering of resolveConstructor to make it work for Object/Function. v1

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

::: js/src/vm/GlobalObject.cpp
@@ +391,5 @@
> +
> +    // Create the constructor.
> +    RootedObject ctor(cx, clasp->spec.createConstructor(cx, key));
> +    if (!ctor)
> +        return false;

Can you add \n after the conditions?

@@ +422,5 @@
>  
> +    // Set the property slot and stash type information, so that what we do
> +    // here is equivalent to initBuiltinConstructor.
> +    global->setConstructorPropertySlot(key, ObjectValue(*ctor));
> +    types::AddTypePropertyId(cx, global, id, ObjectValue(*ctor));

I'd rather this relationship was made explicit via a shared method called by both, unless there is something going to happen here in a later patch.
Attachment #8423559 - Flags: review?(jwalden+bmo) → review+
Attachment #8423560 - Flags: review?(jwalden+bmo) → review+
Attachment #8423561 - Flags: review?(jwalden+bmo) → review+
Attachment #8423562 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8423564 [details] [diff] [review]
Part 10 - Switch Function and Object to ClassSpec. v1

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

Bam
Attachment #8423564 - Flags: review?(jwalden+bmo) → review+
Attachment #8423951 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8423565 [details] [diff] [review]
Part 11 - Move ClassObjectCreationOps and FinishClassInitOps closer to their respective JSClasses. v1

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

You're right, that was all pretty direct slicing and dicing; sorry I missed it on the first look.
Attachment #8423565 - Flags: review?(jwalden+bmo) → review+
(In reply to Luke Wagner [:luke] from comment #18)
> Preexisting, but I think you can pass cx->names().Object directly to
> NewFunction w/o the Rooted.  Same thing for functionAtom below.

I get:

 0:02.20 In file included from /files/mozilla/repos/b/obj-x86_64-apple-darwin13.2.0/js/src/Unified_cpp_js_src7.cpp:15:
 0:02.20 /files/mozilla/repos/b/js/src/jsfun.cpp:618:35: error: no matching function for call to 'NewFunction'
 0:02.20     RootedObject functionCtor(cx, NewFunction(cx, ctor, Function, 1, JSFunction::NATIVE_CTOR, self,
 0:02.20                                   ^~~~~~~~~~~
 0:02.20 /files/mozilla/repos/b/js/src/jsfun.h:511:1: note: candidate function not viable: no known conversion from 'js::ImmutablePropertyNamePtr' (aka 'ImmutableTenuredPtr<js::PropertyName *>') to 'HandleAtom' (aka 'Handle<JSAtom *>') for 7th argument
 0:02.20 NewFunction(ExclusiveContext *cx, HandleObject funobj, JSNative native, unsigned nargs,
 0:02.20 ^
 0:02.47 In file included from /files/mozilla/repos/b/obj-x86_64-apple-darwin13.2.0/js/src/Unified_cpp_js_src7.cpp:93:
 0:02.47 /files/mozilla/repos/b/js/src/jsobj.cpp:90:12: error: no matching function for call to 'NewFunction'
 0:02.47     return NewFunction(cx, ctor, obj_construct, 1, JSFunction::NATIVE_CTOR, self,
 0:02.47            ^~~~~~~~~~~
 0:02.47 /files/mozilla/repos/b/js/src/jsfun.cpp:1767:5: note: candidate function not viable: no known conversion from 'js::ImmutablePropertyNamePtr' (aka 'ImmutableTenuredPtr<js::PropertyName *>') to 'HandleAtom' (aka 'Handle<JSAtom *>') for 7th argument
 0:02.47 js::NewFunction(ExclusiveContext *cx, HandleObject funobjArg, Native native, unsigned nargs,
 0:02.47     ^
Thanks for the reviews luke! Final all-platform try push:

https://tbpl.mozilla.org/?tree=Try&rev=df37d4d9c0c9
(In reply to Bobby Holley (:bholley) from comment #22)
I think that is a compile error b/c C++ would need to do two conversions in a row:
  ImmutableTenuredPtr<PropertyName*> -> Handle<PropertyName*> -> Handle<JSAtom*>
so I think you need to help it along by writing:
  Handle<JSAtom*> objectAtom = cx->names().Object;
which at least avoids the Rooted.
Still doesn't compile. I'm going to drop it, and let you pick it up if you're inspired.
Oops, I should have written HandlePropertyName.  Anyhow, you can just write:

     NewFunction(...,
                 HandlePropertyName(cx->names().Object));

I actually tried compiling this time.
Try push isn't 100% done, but it looks green in the places I would be concerned about.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=eaa9169a7756
Comment on attachment 8423557 [details] [diff] [review]
Part 3 - Streamline __proto__ getter and setter definitions into a JSPropertySpec. v1

>+static bool
>+ProtoSetter(JSContext *cx, unsigned argc, Value *vp)
...
>-static bool
>-ProtoSetterImpl(JSContext *cx, CallArgs args)
...
>-static bool
>-ProtoSetter(JSContext *cx, unsigned argc, Value *vp)
>-{
>-    CallArgs args = CallArgsFromVp(argc, vp);
>-
>-    // Do this here, rather than in |ProtoSetterImpl|, so even likely-buggy
>-    // use of the __proto__ setter on unacceptable values, where no subsequent
>-    // use occurs on an acceptable value, will trigger a warning.
>-    RootedObject callee(cx, &args.callee());
>-    if (!GlobalObject::warnOnceAboutPrototypeMutation(cx, callee))
>-        return false;
>-
>-    return CallNonGenericMethod(cx, TestProtoThis, ProtoSetterImpl, args);
>-}
Does this not regress bug 948227?
Depends on: 1021258
(In reply to neil@parkwaycc.co.uk from comment #29)
> Does this not regress bug 948227?

Good catch! Filed bug 1021258.
Duplicate of this bug: 607863
You need to log in before you can comment on or make changes to this bug.