Closed
Bug 766800
Opened 12 years ago
Closed 12 years ago
add a helper for freeing memory from JSContext/JSRuntime
Categories
(Core :: JavaScript Engine, enhancement)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: Benjamin, Unassigned)
Details
(Whiteboard: [js:t])
Attachments
(1 file)
3.54 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•12 years ago
|
Attachment #635148 -
Flags: review?(jwalden+bmo)
Comment 1•12 years ago
|
||
Comment on attachment 635148 [details] [diff] [review] implementation with a few examples Review of attachment 635148 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/Utility.h @@ +608,5 @@ > }; > SCOPED_TEMPLATE(ScopedDeletePtr, ScopedDeletePtrTraits) > > +template <class T> > +class AutoFree Would a ScopedCxFreePtr class be better?
Comment 2•12 years ago
|
||
ScopedCxFreePtr/ScopedRuntimeFreePtr (using SCOPED_TEMPLATE) sounds like a good idea.
Updated•12 years ago
|
Whiteboard: [js:t]
Reporter | ||
Comment 3•12 years ago
|
||
The problem with reusing Scoped is it has no provision for passing context along with the resource, so it's hard to get cx/runtime there.
Comment 4•12 years ago
|
||
Is it worth adding a variant of SCOPED_TEMPLATE (from mfbt/Scoped.h) that takes a |freer| argument and implementing Scoped{Cx,Runtime}{Free,Delete}Ptr in terms of that? It's a bit odd that we have a mix of AutoFoo and ScopedFoo types for this stuff, but oh well.
Comment 5•12 years ago
|
||
Comment on attachment 635148 [details] [diff] [review] implementation with a few examples Review of attachment 635148 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/Utility.h @@ +607,5 @@ > static void release(T *ptr) { Foreground::delete_(ptr); } > }; > SCOPED_TEMPLATE(ScopedDeletePtr, ScopedDeletePtrTraits) > > +template <class T> class Freer, please -- T's fine for totally generic concepts, but this case isn't really generic at all. @@ +608,5 @@ > }; > SCOPED_TEMPLATE(ScopedDeletePtr, ScopedDeletePtrTraits) > > +template <class T> > +class AutoFree The "Scoped" name came up on the bug that added mfbt/Scoped.h, and for some reason it was deemed better than "Auto". I don't remember why. The reason didn't seem valid or invalid to me, it all just looked like bikeshedding, so I studiously avoided caring. Do we anticipate this being used both for cx->free and for rt->free? That's the only reason I can see to have this be freer-parametrized at all. If we do anticipate that, then the CxFreePtr bit of the change seems unwise for obvious reasons. If we don't, this name (whether Auto or Scoped I don't care) seems fine to me. But I'm not going to argue strenuously for anything particular. @@ +616,5 @@ > + T *freer; > + void *ptr; > + > + public: > + AutoFree(T *freer, void *ptr MOZ_GUARD_OBJECT_NOTIFIER_PARAM) Put the macro param on its own line so it's visually distinct from the actual arguments, like so: AutoFree(T *freer, void *ptr MOZ_GUARD_OBJECT_NOTIFIER_PARAM) @@ +617,5 @@ > + void *ptr; > + > + public: > + AutoFree(T *freer, void *ptr MOZ_GUARD_OBJECT_NOTIFIER_PARAM) > + : freer(freer), ptr(ptr) This should be indented two spaces from the 'A'/'{' column, not four. @@ +618,5 @@ > + > + public: > + AutoFree(T *freer, void *ptr MOZ_GUARD_OBJECT_NOTIFIER_PARAM) > + : freer(freer), ptr(ptr) > + { MOZ_GUARD_OBJECT_NOTIFIER_INIT; } { MOZ_GUARD_OBJECT_NOTIFIER_INIT; } @@ +619,5 @@ > + public: > + AutoFree(T *freer, void *ptr MOZ_GUARD_OBJECT_NOTIFIER_PARAM) > + : freer(freer), ptr(ptr) > + { MOZ_GUARD_OBJECT_NOTIFIER_INIT; } > + ~AutoFree() { freer->free_(ptr); } ~AutoFree() { freer->free_(ptr); }
Attachment #635148 -
Flags: review?(jwalden+bmo) → review+
Reporter | ||
Comment 6•12 years ago
|
||
So, when looking for more places to use this, I discovered AutoReleasePtr in jscntx.h.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•