1071967 - Replace ScopedFreePtr with UniquePtr in jsapi.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Guilherme Gonçalves [:ggp]]

ScopedFreePtr is deprecated in favor of UniquePtr, and UniquePtr gives us the opportunity to specify which deallocation function to use, whereas ScopedFreePtr necessarily uses free() even for memory allocated with js_{m,c,re}alloc.

Comment 1

[Guilherme Gonçalves [:ggp]]

Attachment: Replace ScopedFreePtr with UniquePtr in jsapi.cpp

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8494082 [details] [diff] [review]
Replace ScopedFreePtr with UniquePtr in jsapi.cpp

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

Great! Thanks for fixing that!

Comment 3

[Nicholas Nethercote [inactive]]

> ScopedFreePtr is deprecated in favor of UniquePtr

I understand this is the consensus, and this patch does a fine job of converting.

But I wonder if it makes the code harder to read. The use of UniquePtr suggests two things: (a) it's a unique pointer, and (b) it'll be auto-freed. In this case, only (b) is needed/relevant. Reading the code, I think "oh this is a unique pointer" but then I see that every use of it involves reset() or get() and so then I think "oh, it's not really unique". Hmm.

Comment 4

[Guilherme Gonçalves [:ggp]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=b14fe43da44f

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28fa0ca32114

Comment 6

[Jeff Walden [:Waldo]]

"unique" refers to *ownership*.  UniquePtr is the single owner of whatever pointer it contains.  You can "borrow" a copy using get(), but that doesn't make it any less of a single owner of that pointer.  Automatic destruction is merely a logical consequence of UniquePtr being the single owner of whatever it contains.

reset() is just a helper method to force modification of a UniquePtr to a raw pointer, to use a form other than assignment to make the transfer of ownership clearer.  get() as noted doesn't change ownership, so it has no relevance to the meaning of UniquePtr here.

This would be better if both UTF8CharsToNewTwoByteCharsZ and InflateString returned UniquePtr<char16_t[], JS::FreePolicy>, in that you could use simple assignment instead of reset().  (You could also have a somewhat-cleaner ternary here, too, then.)  But that's mostly a cosmetic thing, when the use is as simple as here.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28fa0ca32114