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

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.)
Posted 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]:
-----------------------------------------------------------------

Cool.

::: 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.

"UniquePtr"?

::: 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+
https://hg.mozilla.org/mozilla-central/rev/cd9057d90944
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.