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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 2

[Jim Blandy :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.

Comment 3

[Luke Wagner [:luke]]

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.

Comment 4

[Jeff Walden [:Waldo]]

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 5

[Luke Wagner [:luke]]

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.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f537219d14fc