Closed
Bug 867341
Opened 9 years ago
Closed 9 years ago
Replace AutoObjectRooter with RootedObject
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: sfink, Assigned: sfink)
Details
Attachments
(3 files)
1.83 KB,
patch
|
smaug
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
9.74 KB,
patch
|
vladan
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
11.90 KB,
patch
|
terrence
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Another boring review for smaug.
Attachment #743803 -
Flags: review?(bugs)
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #743806 -
Flags: review?(terrence)
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
(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.
Updated•9 years ago
|
Attachment #743803 -
Flags: review?(bugs) → review+
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Attachment #743803 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #743805 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #743806 -
Flags: checkin+
Assignee | ||
Comment 8•9 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/e2dc6cdc1c02 http://hg.mozilla.org/integration/mozilla-inbound/rev/e728bef882df http://hg.mozilla.org/integration/mozilla-inbound/rev/0875726579b2
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0875726579b2 https://hg.mozilla.org/mozilla-central/rev/e728bef882df https://hg.mozilla.org/mozilla-central/rev/e2dc6cdc1c02
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in
before you can comment on or make changes to this bug.
Description
•