Closed Bug 511425 Opened 11 years ago Closed 11 years ago

removal of JSObjectOps.(get|set)RequiredSlot

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: igor, Assigned: igor)

References

(Blocks 1 open bug, )

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

With the removal of the liveconnect JSObjectOps.(get|set)RequiredSlot is only defined for native objects. I suggest to remove these methods and simplify code assuming that only native objects can have required slots.
Blocks: 408416
No longer depends on: 408416
Attached patch v1 (obsolete) — Splinter Review
Here is an untested patch. Besides removal of (get|set)RequiredSlot it also changes SLOT macros to insist that the object is native. This avoids useless OBJ_IS_NATIVE check in OBJ_(GET|SET)_SLOT macros since the fast array, the only non-native object, accesses the slots directly.
Attached patch v2 (obsolete) — Splinter Review
This is what I have submitted to the try server.
Attachment #395621 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
Here is the version that compiles and passes try server. Besides changes from the comment 1 it replaces few more jsobj.cpp macroses with inlined function. To keep patch relatively small I have kept (ST)?OBJ_(GET|SET|CLEAR)_(PROTO|PARENT) unless the patch has to touch the code in any case.
Attachment #396236 - Attachment is obsolete: true
Attachment #397096 - Flags: review?(brendan)
Comment on attachment 397096 [details] [diff] [review]
v3

Nice -- but do we need the JS_ALWAYS_INLINE qualifiers on the new JSObject instance methods? I think not, but I defer to experts.

/be
Attachment #397096 - Flags: review?(brendan) → review+
Don't think so, excepting evidence that any of them aren't actually inlined and that matters perf-wise.
Let's lose all the JS_ALWAYS_INLINE noise then. Thanks,

/be
Attached patch v4Splinter Review
In the new version I dropped JS_ALWAYS_INLINE or replaced it with the inline keyword.
Attachment #397096 - Attachment is obsolete: true
Attachment #397209 - Flags: review+
http://hg.mozilla.org/tracemonkey/rev/10380ffe4d49
Whiteboard: fixed-in-tracemonkey
I've argued against inline qualifier on an instance method defined inside its class. It's redundant and it adds noise.

If you have to forward-declare the instance method but it's inline later on in the .h file (to use later definitions, say), then the inline is a useful clue.

/be
+/* On failure the function unlocks the object. */
+static bool
+ReservedSlotIndexOK(JSContext *cx, JSObject *obj, JSClass *clasp,
+                    uint32 index, uint32 limit)
+{
+    JS_IS_OBJ_LOCKED(cx, obj);

Is that last line supposed to be an assertion? gcc warns that the statement has no effect.
(In reply to comment #10)
> Is that last line supposed to be an assertion? gcc warns that the statement has
> no effect.

Thanks for spotting this, I fixed this in the follow up:

http://hg.mozilla.org/tracemonkey/rev/b893905b0e0f
http://hg.mozilla.org/mozilla-central/rev/10380ffe4d49
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 521124
You need to log in before you can comment on or make changes to this bug.