Closed Bug 1103152 Opened 10 years ago Closed 9 years ago

Remove some JSClass stub functions

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(2 files)

I want to make all JSClass function pointer members nullable. This is the first step. I'll make these nullable:

    addProperty
    delProperty
    enumerate
    resolve
    convert

and I'll remove the corresponding stub functions JS_DeletePropertyStub, JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub. (addProperty uses JS_PropertyStub which can't be removed yet.)

After this, only getProperty and setProperty will remain.
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Comment on attachment 8527106 [details] [diff] [review]
Remove JS_DeletePropertyStub, JS_EnumerateStub, JS_ResolveStub, and JS_ConvertStub. Make five mandatory JSClass hooks optional (nullable)

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

Shifting review. I really want to land this and the patches in bug 1103368 as soon as I can. They're going to bitrot like crazy.
Attachment #8527106 - Flags: review?(jwalden+bmo) → review?(bhackett1024)
Comment on attachment 8527106 [details] [diff] [review]
Remove JS_DeletePropertyStub, JS_EnumerateStub, JS_ResolveStub, and JS_ConvertStub. Make five mandatory JSClass hooks optional (nullable)

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

Nice!!!!

::: js/src/jsapi.cpp
@@ +2143,5 @@
>      return true;
>  }
>  
>  JS_PUBLIC_API(bool)
> +JS::OrdinaryToPrimitive(JSContext *cx, HandleObject obj, JSType type, MutableHandleValue vp)

OrdinaryConvert, or OrdinaryDefaultValue?
Attachment #8527106 - Flags: review?(bhackett1024) → review+
(In reply to Brian Hackett (:bhackett) from comment #4)
> OrdinaryConvert, or OrdinaryDefaultValue?

The algorithm in the spec is named OrdinaryToPrimitive:

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-toprimitive (scroll down a little there)

Beyond that, this entire mechanism is going to more or less go away when we implement conversion-to-primitive using a @@toPrimitive symbol, so it's not worth polishing things (like renaming the field from convert to toPrimitive or similar).
(In reply to Brian Hackett (:bhackett) from comment #4)
> > +JS::OrdinaryToPrimitive(JSContext *cx, HandleObject obj, JSType type, MutableHandleValue vp)
> 
> OrdinaryConvert, or OrdinaryDefaultValue?

Waldo's right, the name just follows the spec.

I admit, I did think "this name sucks" as I typed it in.

Added a comment to help explain:

    /*
     * Implements ES6 draft rev 28 (2014 Oct 14) 7.1.1, second algorithm.
     *
     * Most users should not call this -- use JS::ToNumber, ToBoolean, or ToString
     * instead. This should only be called from custom convert hooks. It implements
     * the default conversion behavior shared by most objects in JS, so it's useful
     * as a fallback.
     */
    extern JS_PUBLIC_API(bool)
    OrdinaryToPrimitive(JSContext *cx, HandleObject obj, JSType type, MutableHandleValue vp);
14:20:40     INFO -  {standard input}: Assembler messages:
14:20:40     INFO -  {standard input}:49744: Error: can't resolve `_ZN2js16TypedArrayObject7classesE' {*UND* section} - `.LPIC442' {*UND* section}
14:20:40    ERROR -  make[6]: *** [Unified_cpp_js_src8.o] Error 1
14:20:40     INFO -  make[6]: *** Waiting for unfinished jobs....

**Assembler**?!

No clue what's going on here.
Try-bisecting continues apace. The good news is, it looks like 3 of the 5 hooks can be nulled out without angering the fickle Ice Cream Sandwich gods. I'm guessing 4 would be fine. Still no idea what's really causing this.
Re: comment 27, I gave up and #ifdef'd some code just for GCC 4.4.

If that works, let's land it and move on...
Attachment #8531447 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/a7d403088a16
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: