557713 - encapsulate JSSLOT_ARGS_* within JSObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch encapsulates all fslots[JSSLOT_ARGS_*] accesses.  It:

- Moves JSSLOT_ARGS_* into JSObject and makes them private.  Except for
  JSSLOT_ARGS_START which is no longer necessary (actual arguments go in
  dslots) and is removed.

- Moves ARGS_FIXED_RESERVED_SLOTS into JSObject but leaves it public;  it's
  required for JSClass::flags.  That's a bit ugly but fodder for a separate
  patch (also, JSClass is in jsapi.h which makes thing harder).

- Changes:
  - IsOverriddenArgsLength(obj)   to  obj->isArgsLengthOverridden()
  - GetArgsLength(obj)            to  obj->getArgsLength()
  - InitArgsLengthSlot(obj)       to  obj->setArgsLength()
  - SetOverriddenArgsLength(obj)  to  obj->markArgsLengthOverridden()

  I felt these names were more consistent with each other and other JSObject
  methods that the old ones.

- Adds {get,set}ArgsCallee().

- Removes a now-unnecessary assertion from args_reserveSlots().

- Slightly reorders some array getters/setters.

Comment 1

[Brendan Eich [:brendan]]

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

> args_delProperty(JSContext *cx, JSObject *obj, jsval idval, jsval *vp)
> {
>     JS_ASSERT(obj->isArguments());
> 
>     if (JSVAL_IS_INT(idval)) {
>         uintN arg = uintN(JSVAL_TO_INT(idval));
>-        if (arg < GetArgsLength(obj))
>+        if (arg < obj->getArgsLength())
>             SetArgsSlot(obj, arg, JSVAL_HOLE);
>     } else if (idval == ATOM_KEY(cx->runtime->atomState.lengthAtom)) {
>-        SetOverriddenArgsLength(obj);
>+        obj->markArgsLengthOverridden();

Nicely abstracted. Minor comment: "mark" works metaphorically, but it is also a GC verb. Alternatives: "set" (as originally), "note". Your call.

How about abstracting the use of JSVAL_HOLE to mark an arguments element overridden? You could have "mark" and "is" methods taking the index and hiding all the hole ugliness.

>+    /*
>+     * Reserved slot structure for Arguments objects:
>+     *
>+     * JSSLOT_PRIVATE       - the corresponding frame until the frame exits.
>+     * JSSLOT_ARGS_LENGTH   - the number of actual arguments and a flag
>+     *                        indicating whether arguments.length was
>+     *                        overwritten.
>+     * JSSLOT_ARGS_CALLEE   - the arguments.callee value or JSVAL_HOLE if that
>+     *                        was overwritten.
>+     *
>+     * Argument index i is stored in dslots[i].  But future-proof your code by
>+     * using {Get,Set}ArgsSlot instead of naked dslots references.
>+     */
>+  private:
>+    static const uint32 JSSLOT_ARGS_LENGTH = JSSLOT_PRIVATE + 1;
>+    static const uint32 JSSLOT_ARGS_CALLEE = JSSLOT_PRIVATE + 2;
>+
>+  public:
>+    /* Number of extra fixed slots besides JSSLOT_PRIVATE. */
>+    static const uint32 ARGS_FIXED_RESERVED_SLOTS = 2;

This is almost useless. I mean it has one use, exactly ;-). But the value must be 2, based on the two private static consts just above. In that light would hard-coding 2 in the JSClass instance's flags initializer, and maybe (statically, if possible) asserting be worse? It would separate the 2 in the class init from this code.

Another idea: a static inline method that returns this manifest, or just JSSLOT_ARGS_CALLEE - JSSLOT_PRIVATE. Assuming it would be called in a POD static/extern initializer.

Looks great otherwise, r=me with above taken into account as you see fit. Thanks, and keep 'em coming!

/be

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> >+  private:
> >+    static const uint32 JSSLOT_ARGS_LENGTH = JSSLOT_PRIVATE + 1;
> >+    static const uint32 JSSLOT_ARGS_CALLEE = JSSLOT_PRIVATE + 2;
> >+
> >+  public:
> >+    /* Number of extra fixed slots besides JSSLOT_PRIVATE. */
> >+    static const uint32 ARGS_FIXED_RESERVED_SLOTS = 2;
> 
> This is almost useless. I mean it has one use, exactly ;-). But the value must
> be 2, based on the two private static consts just above. In that light would
> hard-coding 2 in the JSClass instance's flags initializer, and maybe
> (statically, if possible) asserting be worse? It would separate the 2 in the
> class init from this code.
> 
> Another idea: a static inline method that returns this manifest, or just
> JSSLOT_ARGS_CALLEE - JSSLOT_PRIVATE. Assuming it would be called in a POD
> static/extern initializer.

Some of the class kinds have this RESERVED_SLOTS value, and some just hard-code a number.  I was leaning towards using RESERVED_SLOTS values everywhere because it's defined right next to the JSSLOT_* values -- so if you added a new JSSLOT_* value for a class you'd be more likely to remember to change the RESERVED_SLOT value.  And it seems simpler than either a static assertion or an inline method?

> Looks great otherwise, r=me with above taken into account as you see fit.
> Thanks, and keep 'em coming!

No problem, I've got a big dslots-encapsulation patch in the works :)

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> 
> How about abstracting the use of JSVAL_HOLE to mark an arguments element
> overridden? You could have "mark" and "is" methods taking the index and hiding
> all the hole ugliness.

That relates to the dslots and this bug involves the fslots, so I filed it as a follow-up: bug 558706.

Comment 4

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a39e0ce1c8ca