Closed Bug 793588 Opened 12 years ago Closed 12 years ago

Exactly root jsiter.{h,cpp}

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [js:t])

Attachments

(2 files)

We need to exactly root jsiter.{h,cpp}
We don't need this parameter.
Attachment #663921 - Flags: review?(sphink)
Nothing too surprising here.
Attachment #663925 - Flags: review?(sphink)
Comment on attachment 663925 [details] [diff] [review]
(part 2) - Exactly root jsiter.{cpp,h}.

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

::: js/src/jsinterp.cpp
@@ +1779,5 @@
>  {
>      JS_ASSERT(regs.sp[-1].isObject());
>      PUSH_NULL();
>      MutableHandleValue res = MutableHandleValue::fromMarkedLocation(&regs.sp[-1]);
> +    RootedObject obj(cx, &regs.sp[-2].toObject());

Drive-by comment: other interpreter cases use rootObject0.
Comment on attachment 663925 [details] [diff] [review]
(part 2) - Exactly root jsiter.{cpp,h}.

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

::: js/src/jsinterp.cpp
@@ +1779,5 @@
>  {
>      JS_ASSERT(regs.sp[-1].isObject());
>      PUSH_NULL();
>      MutableHandleValue res = MutableHandleValue::fromMarkedLocation(&regs.sp[-1]);
> +    RootedObject obj(cx, &regs.sp[-2].toObject());

Yes, within the interp loop it's better to use those here and in the other switch cases.

::: js/src/jsiter.cpp
@@ +811,5 @@
>      JS_FN("next",      iterator_next,       0, 0),
>      JS_FS_END
>  };
>  
> +static RawObject

Not for a return value.

::: js/src/jswrapper.cpp
@@ +634,5 @@
>      AutoCloseIterator(JSContext *cx, JSObject *obj) : cx(cx), obj(obj) {}
>  
> +    ~AutoCloseIterator() {
> +        if (obj) {
> +            RootedObject rObj(cx, obj);

Could you make the obj member be a Rooted instead? (I guess if obj is often NULL, then the way you have it here is better.)
Attachment #663925 - Flags: review?(sphink) → review+
Comment on attachment 663921 [details] [diff] [review]
(part 1) - Remove vacuous JSObject* parameter to Enumerate() et al.

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

I tried to dig in a little to see if Enumerate might want to have both the original object as well as the current object being examined (possibly an n-th degree prototype of the original object), but all I could conclude is that the static Enumerate is confusingly named, and is more something like IsIdInEnumeration. Or something. At any rate, I saw no reason to worry about this being reversed anytime soon.
Attachment #663921 - Flags: review?(sphink) → review+
> Yes, within the interp loop it's better to use those here and in the other switch cases.

Sorry, thought Splinter would show both comments. This was in agreement with Jan's comment that you should use rootObject0 within the interp loop. (They don't need to be constantly added and removed from the list, though admittedly they do run the risk of holding onto garbage occasionally.)
https://hg.mozilla.org/mozilla-central/rev/821a32229045
https://hg.mozilla.org/mozilla-central/rev/c9b1f0ce3779
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: