Closed
Bug 793588
Opened 12 years ago
Closed 12 years ago
Exactly root jsiter.{h,cpp}
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: [js:t])
Attachments
(2 files)
6.82 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
33.79 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
We need to exactly root jsiter.{h,cpp}
Assignee | ||
Comment 1•12 years ago
|
||
We don't need this parameter.
Attachment #663921 -
Flags: review?(sphink)
Assignee | ||
Comment 2•12 years ago
|
||
Nothing too surprising here.
Attachment #663925 -
Flags: review?(sphink)
Comment 3•12 years ago
|
||
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(®s.sp[-1]); > + RootedObject obj(cx, ®s.sp[-2].toObject()); Drive-by comment: other interpreter cases use rootObject0.
Comment 4•12 years ago
|
||
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(®s.sp[-1]); > + RootedObject obj(cx, ®s.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 5•12 years ago
|
||
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+
Comment 6•12 years ago
|
||
> 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.)
Assignee | ||
Comment 7•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/821a32229045 https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b1f0ce3779
Comment 8•12 years ago
|
||
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.
Description
•