Closed Bug 1037106 Opened 5 years ago Closed 5 years ago

Use UniquePtr to manage ownership of the Debugger* for a Debugger object, prior to being stored in a private slot

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: Waldo, Assigned: Waldo)

Details

Attachments

(1 file)

It's very slightly sad we have to release into a raw pointer, then store into the private slot, rather than do both at once.  But we need the pointer for the subsequent calls, and UniquePtr::release() nulls out the UniquePtr.  So this seems the cleanest we can do.
Attached patch PatchSplinter Review
Attachment #8453980 - Flags: review?(jimb)
Comment on attachment 8453980 [details] [diff] [review]
Patch

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

r=me on the Debugger.cpp bits. I'd like to see what luke thinks about the MAKE stuff.

::: js/public/Utility.h
@@ +170,5 @@
>  #define JS_NEW_BODY(allocator, t, parms)                                       \
>      void *memory = allocator(sizeof(t));                                       \
>      return memory ? new(memory) t parms : nullptr;
> +#define JS_MAKE_BODY(newname, T, parms)                                        \
> +    T *ptr = newname<T> parms;                                                    \

It looks like your backslashes are not lined up here.
Attachment #8453980 - Flags: review?(luke)
Attachment #8453980 - Flags: review?(jimb)
Attachment #8453980 - Flags: review+
This is indeed cool.  My only nit to pick would be the name "make_".  For one thing, the trailing underscore seems unnecessary.  If all the sibling names had trailing _ then it'd make sense, but there are plenty that don't (pod_malloc, et al).

For another thing, what "make" does isn't obvious from its name, nor will it be from local syntax (viz., in "auto x = cx->make_<T>();").  It'll take a bit of grepping (frustrated by macros) to find the source.  I'd rather, at the expense of verbosity, go with new_unique.  One benefit is that having x_unique mean "like x, but return a UniquePtr instead" extends to the other helpers, e.g., pod_malloc_unique.
The "make" name is a nod to std::make_unique, mozilla::MakeUnique.  Maybe make_unique then?  (Or do away with this nonsense, out-of-place underscore naming and do makeUnique?)
Comment on attachment 8453980 [details] [diff] [review]
Patch

Ohhhh.  How far I have fallen behind the times :)  makeUnique or make_unique seems quite reasonable; make_unique would be a bit more symmetric with pod_malloc et al.
Attachment #8453980 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/f537219d14fc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.