638347 - JS_Enumerate2, remove JS_NewPropertyIterator, JSAutoIdArray

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

This patch adds JS_Enumerate2, a proper JS API way of getting more than just own properties and also non-enumerable properties. Since JS_Enumerate*() is about to do its own rooting, all that JSAutoIdArray needs to do these days is call JS_DestroyIdArray and it is not a rooter any more. I moved it into JSAPI so people don't include our guts any more. JS_NewPropertyIterator is a really bad API that was only used in our shell and CTypes. I replaced it with JS_Enumerate. There is really no reason to keep it, and it holds shapes across API calls which I am really not brave enough to do.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Another API removal, so Brendan it is.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 516498 [details] [diff] [review]
patch

> JS_Enumerate(JSContext *cx, JSObject *obj)
> {
>+    return JS_Enumerate2(cx, obj, JS_ENUM_OWNONLY);
>+}
>+
>+JS_PUBLIC_API(JSIdArray *)
>+JS_Enumerate2(JSContext *cx, JSObject *obj, uintN flags)

No more -2, or -Ex, or any such name suffixes if you please. JS_EnumerateIds would be better, it puts a noun after the verb.

>-/*
>- * XXX reverse iterator for properties, unreverse and meld with jsinterp.c's
>- *     prop_iterator_class somehow...
>- * + preserve the obj->enumerate API while optimizing the native object case
>- * + native case here uses a Shape *, but that iterates in reverse!
>- * + so we make non-native match, by reverse-iterating after JS_Enumerating
>- */
>-const uint32 JSSLOT_ITER_INDEX = 0;
>-
>-static void
>-prop_iter_finalize(JSContext *cx, JSObject *obj)

Great to see this cruft go.

>     enum {
>         JSVAL =        -1, /* js::AutoValueRooter */
>         SHAPE =        -2, /* js::AutoShapeRooter */
>         PARSER =       -3, /* js::Parser */
>         SCRIPT =       -4, /* js::AutoScriptRooter */
>         ENUMERATOR =   -5, /* js::AutoEnumStateRooter */
>-        IDARRAY =      -6, /* js::AutoIdArray */
>-        DESCRIPTORS =  -7, /* js::AutoPropDescArrayRooter */
>-        NAMESPACES =   -8, /* js::AutoNamespaceArray */
>-        XML =          -9, /* js::AutoXMLRooter */
>-        OBJECT =      -10, /* js::AutoObjectRooter */
>-        ID =          -11, /* js::AutoIdRooter */
>-        VALVECTOR =   -12, /* js::AutoValueVector */
>-        DESCRIPTOR =  -13, /* js::AutoPropertyDescriptorRooter */
>-        STRING =      -14, /* js::AutoStringRooter */
>-        IDVECTOR =    -15, /* js::AutoIdVector */
>-        BINDINGS =    -16, /* js::Bindings */
>-        SHAPEVECTOR = -17  /* js::AutoShapeVector */
>+        DESCRIPTORS =  -6, /* js::AutoPropDescArrayRooter */
>+        NAMESPACES =   -7, /* js::AutoNamespaceArray */
>+        XML =          -8, /* js::AutoXMLRooter */
>+        OBJECT =       -9, /* js::AutoObjectRooter */
>+        ID =          -10, /* js::AutoIdRooter */
>+        VALVECTOR =   -11, /* js::AutoValueVector */
>+        DESCRIPTOR =  -12, /* js::AutoPropertyDescriptorRooter */
>+        STRING =      -13, /* js::AutoStringRooter */
>+        IDVECTOR =    -14, /* js::AutoIdVector */
>+        BINDINGS =    -15, /* js::Bindings */
>+        SHAPEVECTOR = -16  /* js::AutoShapeVector */

Since you are renumbering after the removal, please clean up a bit: put ID right before STRING before OBJECT, DESCRIPTOR right before DESCRIPTORS, IDVECTOR right before VALVECTOR before SHAPEVECTOR, NAMESPACES and XML at the end, and order totally as this sentence does.

Pre-existing style nit, fix it or don't: lining up the = helps readability and is done elsewhere.

>-class AutoIdArray : private AutoGCRooter {

Moving this to the public API should not impose JS- prefixing on all existing uses. Make a typedef here for compatibility.

Moreover, losing AutoGCRooter loses this:

>-    /* No copy or assignment semantics. */
>-    AutoIdArray(AutoIdArray &ida);
>-    void operator=(AutoIdArray &ida);

Add the equivalent protection back in jsapi.h's JSAutoIdArray if you can.

> static bool
> DefineProperties(JSContext *cx, JSObject *obj, JSObject *props)
> {
>-    AutoIdArray ida(cx, JS_Enumerate(cx, props));
>+    JSAutoIdArray ida(cx, JS_Enumerate(cx, props));

Again, none of these uses should have to grow a JS- prefix.

/be

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 516498 [details] [diff] [review]
patch

>+    bool operator! () {
>+        return !!mIdArray;

Was the patch tested? If mIdArray is null, !mIdArray is true, and !!mIdArray is false, and operator! returns false -- so a test of the form "if (!ida)" tests false on OOM during construction, not true. Sanity check: existing AutoIdArray has:

    bool operator!() {
        return idArray == NULL;
    }

We usually test !ptr not ptr == NULL but here an exception may help avoid confusion, since this is implementing the ! operator and we might like that to bottom out in a different operator on POD.

Comment 4

[Andreas Gal :gal]

Comment on attachment 516498 [details] [diff] [review]
patch

Will fix and try-server running.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 516498 [details] [diff] [review]
patch

>+class JSAutoIdArray {
>+  public:
>+    JSAutoIdArray(JSContext *cx, JSIdArray *ida JS_GUARD_OBJECT_NOTIFIER_PARAM)
>+        : mContext(cx), mIdArray(ida) {

Nit: the : and member ctor-init thingies should be half-indented here, per house style (a few exceptions -- JSAuto*Request, JSAutoStructuredCloneBuffer -- in jsapi.h have crept in; fix for bonus karma).

{ on its own line would be better due to the "multiline ctor head" (similar to multiline if condition style rule, although the two-space half-indent helps relieve the "where does the body start" confusion here).

/be

Comment 6

[Terrence Cole [:terrence]]

This code has all changed rather dramatically in the last 5 years. Also, we should just fix JS_Enumerate rather than adding another API for this.