1036703 - Manage TokenStream::{sourceMap,display}URL_ using UniquePtr.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Bug 953296 just implemented mozilla::UniquePtr -- a smart pointer that manages sole ownership of a resource.  UniquePtr can't be copied, only moved from one location to another, and on destruction it frees the resource it manages.  There's a small escape hatch that exposes the actual resource -- get() -- but excepting that it manages ownership just like you'd want it to.

This bug adds a couple UniquePtr instances into TokenStream, used for owning a source map URL and display URL.  This switch lets us get rid of several manual frees, including one (in getDirective) that requires careful knowledge of token stream state handling to understand its necessity.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Manage TokenStream::{sourceMap,display}URL_ using UniquePtr

Comment 2

[Jim Blandy :jimb]

Comment on attachment 8453444 [details] [diff] [review]
Manage TokenStream::{sourceMap,display}URL_ using UniquePtr

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

Looks good to me.

::: js/src/frontend/TokenStream.cpp
@@ +891,4 @@
>          if (!*destination)
>              return false;
>  
> +        PodCopy((*destination).get(), tokenbuf.begin(), length);

This isn't 'destination->get()'? After all, destination *is* a plain pointer.

::: js/src/frontend/TokenStream.h
@@ +857,5 @@
>      bool getDirectives(bool isMultiline, bool shouldWarnDeprecated);
>      bool getDirective(bool isMultiline, bool shouldWarnDeprecated,
>                        const char *directive, int directiveLength,
> +                      const char *errorMsgPragma,
> +                      mozilla::UniquePtr<jschar[], JS::FreePolicy> *destination);

Would it help to introduce a typedef for this? I like being able to know how it will behave without looking anything up, but I'm also slowed down scanning all that to the end and looking for surprises.

::: js/src/vm/MallocProvider.h
@@ +108,5 @@
>  
>      template <class T>
> +    mozilla::UniquePtr<T[], JS::FreePolicy> make_pod_array(size_t numElems) {
> +        return mozilla::UniquePtr<T[], JS::FreePolicy>(pod_malloc<T>(numElems));
> +    }

The only difference between this and pod_malloc is the UniquePtr, but the name doesn't reflect that very well.

@@ +112,5 @@
> +    }
> +
> +    template <class T>
> +    T *
> +    pod_calloc(size_t numElems, JSCompartment *comp = nullptr, JSContext *cx = nullptr) {

The style in the rest of this file is to put the return type and the function name on the same line.

Comment 3

[Jeff Walden [:Waldo]]

(In reply to Jim Blandy :jimb from comment #2)
> This isn't 'destination->get()'? After all, destination *is* a plain pointer.

Yeah, changed.  I think I had some mini-iterations where that wasn't possible for some reason or other.

> > +                      const char *errorMsgPragma,
> > +                      mozilla::UniquePtr<jschar[], JS::FreePolicy> *destination);
> 
> Would it help to introduce a typedef for this?

Sort of, but I think we probably want UniquePtr to become ubiquitous/well-known enough that it's not an issue to have no typedef.

> >      template <class T>
> > +    mozilla::UniquePtr<T[], JS::FreePolicy> make_pod_array(size_t numElems) {
> > +        return mozilla::UniquePtr<T[], JS::FreePolicy>(pod_malloc<T>(numElems));
> > +    }
> 
> The only difference between this and pod_malloc is the UniquePtr, but the
> name doesn't reflect that very well.

We want "pod" here to imply that things have to be just data, no initialization of them happening here.  We want "array" because we're returning a T[]-looking UniquePtr, which provides operator[], disables mutation with U* values that convert to T*.

And we want "make" because it's a nod to UniquePtr/unique_ptr idioms.  There's a function std::make_unique<T>(...), mozilla::MakeUnique<T>(...), that news up a T from the provided arguments, then returns it in a UniquePtr.  (And also std::make_unique<T[]>(size_t) and mozilla::MakeUnique<T[]>(size_t).)  So "make" makes good sense here.  ("unique" might be better, but these names are rather long already, or so it seems to me.)

The overall name is not exactly nice.  But there's a modicum of sense to it.

> The style in the rest of this file is to put the return type and the
> function name on the same line.

For just plain pointer maybe that works, but UniquePtr types get unwieldy long enough that I think it makes it rather harder to read that way.  Also, I think we're going to need to break that tradition at some point for this:

    template <class T>
    mozilla::UniquePtr<T[], JS::FreePolicy>
    make_zeroed_pod_array(size_t numElems,
                          JSCompartment *comp = nullptr,
                          JSContext *cx = nullptr)

which seems totally unmanageable if strung onto one line (or one line ending at the first argument).

Comment 4

[Jim Blandy :jimb]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> > The style in the rest of this file is to put the return type and the
> > function name on the same line.
> 
> For just plain pointer maybe that works, but UniquePtr types get unwieldy
> long enough that I think it makes it rather harder to read that way.  Also,
> I think we're going to need to break that tradition at some point for this:
> 
>     template <class T>
>     mozilla::UniquePtr<T[], JS::FreePolicy>
>     make_zeroed_pod_array(size_t numElems,
>                           JSCompartment *comp = nullptr,
>                           JSContext *cx = nullptr)
> 
> which seems totally unmanageable if strung onto one line (or one line ending
> at the first argument).

I was going to suggest doing exactly what you show above. But your patch puts 'T *' on a line all by itself, and that seems silly.

Comment 5

[Jeff Walden [:Waldo]]

Oops, I thought I'd put it across two lines.  Made both cases consistent at two lines, will probably go and make 'em all two lines at some point once the other uniqueptr-ing gets added here.

https://hg.mozilla.org/integration/mozilla-inbound/rev/106cb7c6591a

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/106cb7c6591a