Closed Bug 1036710 Opened 9 years ago Closed 9 years ago

Use UniquePtr to manage ownership of the array of PNK_CASE nodes in table switches. NOT REVIEWED YET


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)



(1 file, 1 obsolete file)

Bug 953296 just landed UniquePtr, a smart pointer that handles single ownership of a resource, freeing it (using the custom means selected) on ultimate destruction, or handing off ownership if the smart pointer is moved.  (UniquePtr isn't copyable, so no worries about duplicate ownership.)

This patch straightforwardly uses UniquePtr to manage the array of PNK_CASE nodes tracked when emitting a JSOP_TABLESWITCH.  The nice thing is we can have the allocator method *return* a UniquePtr so that it's impossible to screw things up unless the result of get() is misused, or unless a release() isn't correctly freed.  (Of course in this case neither concern is an issue, but in general it might be.)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8453450 - Flags: review?(jorendorff)
Attachment #8453450 - Attachment is obsolete: true
Attachment #8453450 - Flags: review?(jorendorff)
Attachment #8453971 - Flags: review?(jorendorff)
Comment on attachment 8453971 [details] [diff] [review]
Slightly better function naming

Review of attachment 8453971 [details] [diff] [review]:


::: js/src/frontend/BytecodeEmitter.cpp
@@ +2748,4 @@
>          /*
>           * Use malloc to avoid arena bloat for programs with many switches.
> +         * Ptr takes care of freeing it on exit.


::: js/src/vm/MallocProvider.h
@@ +127,5 @@
> +    make_zeroed_pod_array(size_t numElems,
> +                          JSCompartment *comp = nullptr,
> +                          JSContext *cx = nullptr)
> +    {
> +        return mozilla::UniquePtr<T[], JS::FreePolicy>(pod_calloc<T>(numElems, comp, cx));

Huh. We have a template that does IsPod<T>, so it seems weird that pod_malloc and pod_calloc don't static_assert that the type being allocated really is a POD type. Feel free to add the assertion... if it compiles...
Attachment #8453971 - Flags: review?(jorendorff) → review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.