Closed Bug 867341 Opened 6 years ago Closed 6 years ago

Replace AutoObjectRooter with RootedObject

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: sfink, Assigned: sfink)

Details

Attachments

(3 files)

AutoObjectRooter is a bit of a footgun in the moving GC world, since it doesn't provide Handles, and all uses are replaceable with RootedObject now.
Another boring review for smaug.
Attachment #743803 - Flags: review?(bugs)
This should be pretty straightforward, but:

Basic rooting guide is at https://developer.mozilla.org/en-US/docs/SpiderMonkey/GC_Rooting_Guide

More detailed documentation is in the source, see http://mxr.mozilla.org/mozilla-central/source/js/public/RootingAPI.h
Attachment #743805 - Flags: review?(vdjeric)
Comment on attachment 743806 [details] [diff] [review]
Remove AutoObjectRooter, replace with RootedObject where needed

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

Looks good.

::: js/src/jsapi.h
@@ -116,5 @@
>          PARSER =       -3, /* js::frontend::Parser */
>          SHAPEVECTOR =  -4, /* js::AutoShapeVector */
>          IDARRAY =      -6, /* js::AutoIdArray */
>          DESCRIPTORS =  -7, /* js::AutoPropDescArrayRooter */
> -        OBJECT =       -8, /* js::AutoObjectRooter */

\o/
Attachment #743806 - Flags: review?(terrence) → review+
Comment on attachment 743805 [details] [diff] [review]
Replace AutoObjectRooter with RootedObject

>   const size_t count = h->bucket_count();
>-  JSObject *rarray = JS_NewArrayObject(cx, count, nullptr);
>+  JS::Rooted<JSObject*> rarray(cx, JS_NewArrayObject(cx, count, nullptr));
>   if (!rarray) {
>     return REFLECT_FAILURE;
>   }

Nitpicks:
- Let's use NULL and nullptr consistently in the changed lines
- Why not RootedObject instead of JS::Rooted<JSObject*>?
Attachment #743805 - Flags: review?(vdjeric) → review+
(In reply to Vladan Djeric (:vladan) from comment #5)
> Comment on attachment 743805 [details] [diff] [review]
> Replace AutoObjectRooter with RootedObject
> 
> >   const size_t count = h->bucket_count();
> >-  JSObject *rarray = JS_NewArrayObject(cx, count, nullptr);
> >+  JS::Rooted<JSObject*> rarray(cx, JS_NewArrayObject(cx, count, nullptr));
> >   if (!rarray) {
> >     return REFLECT_FAILURE;
> >   }
> 
> Nitpicks:
> - Let's use NULL and nullptr consistently in the changed lines

Consistently? I'm not sure what you mean. The file currently uses a mixture; I (or rather, my emacs macro) just preserved whatever was there.

Are you ok with me switching all changed lines to nullptr? Or for that matter, I could change everything in this file to nullptr if you'd like.

> - Why not RootedObject instead of JS::Rooted<JSObject*>?

Current policy is to use the template form everywhere outside of the JS engine and XPConnect. And we may eliminate the typedefs (eg RootedObject) entirely at some point.
Attachment #743803 - Flags: review?(bugs) → review+
(In reply to Steve Fink [:sfink] from comment #6)
> (In reply to Vladan Djeric (:vladan) from comment #5)
> > Nitpicks:
> > - Let's use NULL and nullptr consistently in the changed lines
> 
> Consistently? I'm not sure what you mean. The file currently uses a mixture;
> I (or rather, my emacs macro) just preserved whatever was there.
>
> Are you ok with me switching all changed lines to nullptr? Or for that
> matter, I could change everything in this file to nullptr if you'd like.

I was suggesting using nullptr in the changed lines, but you can change the whole file too -- the whole-file change should be a separate commit.
Attachment #743803 - Flags: checkin+
Attachment #743805 - Flags: checkin+
Attachment #743806 - Flags: checkin+
You need to log in before you can comment on or make changes to this bug.