1030922 - Replace nsAutoPtr with UniquePtr in LayerScope.cpp

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

Right now this code works because our |operator delete| calls through to moz_free. We shouldn't rely on that though.

http://hg.mozilla.org/mozilla-central/file/aab918af0f9c/gfx/layers/LayerScope.cpp#l467

We could use nsAutoArrayPtr, but at this point we might as well wait for the last patch for bug 953296 to get review and use UniquePtr.

Comment 1

[Boris Chiou [:boris]]

(In reply to Jonathan Watt [mostly offline until July 28] from comment #0)
> Right now this code works because our |operator delete| calls through to
> moz_free. We shouldn't rely on that though.
> 
> http://hg.mozilla.org/mozilla-central/file/aab918af0f9c/gfx/layers/
> LayerScope.cpp#l467
> 
> We could use nsAutoArrayPtr, but at this point we might as well wait for the
> last patch for bug 953296 to get review and use UniquePtr.

Hello, I am revising LayerScope.cpp now and can replace all the pointers with UniquePtr.

Comment 2

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #1)
> (In reply to Jonathan Watt [mostly offline until July 28] from comment #0)
> > Right now this code works because our |operator delete| calls through to
> > moz_free. We shouldn't rely on that though.
> > 
> > http://hg.mozilla.org/mozilla-central/file/aab918af0f9c/gfx/layers/
> > LayerScope.cpp#l467
> > 
> > We could use nsAutoArrayPtr, but at this point we might as well wait for the
> > last patch for bug 953296 to get review and use UniquePtr.
> 
> Hello, I am revising LayerScope.cpp now and can replace all the pointers
> with UniquePtr.

Comment 3

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #2)
> (In reply to Boris Chiou [:boris] from comment #1)
> > (In reply to Jonathan Watt [mostly offline until July 28] from comment #0)
> > > Right now this code works because our |operator delete| calls through to
> > > moz_free. We shouldn't rely on that though.
> > > 
> > > http://hg.mozilla.org/mozilla-central/file/aab918af0f9c/gfx/layers/
> > > LayerScope.cpp#l467
> > > 
> > > We could use nsAutoArrayPtr, but at this point we might as well wait for the
> > > last patch for bug 953296 to get review and use UniquePtr.
> > 
> > Hello, I am revising LayerScope.cpp now and can replace all the pointers
> > with UniquePtr.

By the way, there is no std::auto_ptr in LayerScope.cpp now because of Bug 991227, so I changed the title.

Comment 4

[Boris Chiou [:boris]]

Attachment: Replace nsAuto(Array)Ptr with UniquePtr in LayerScope.cpp

Comment 5

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #4)
> Created attachment 8460004 [details] [diff] [review]
> Replace nsAuto(Array)Ptr with UniquePtr in LayerScope.cpp

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=2956dcacfe11

Comment 6

[u480271]

Comment on attachment 8460004 [details] [diff] [review]
Replace nsAuto(Array)Ptr with UniquePtr in LayerScope.cpp

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

From reading the documentation at http://dxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h#30, this patch LGTM.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56e92647d661

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/56e92647d661

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8460004 [details] [diff] [review]
Replace nsAuto(Array)Ptr with UniquePtr in LayerScope.cpp

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

::: gfx/layers/LayerScope.cpp
@@ +380,5 @@
>          if (!WebSocketHelper::GetSocketManager())
>              return true;
>  
>          uint32_t size = aPacket.ByteSize();
> +        UniquePtr<uint8_t[]> data(new uint8_t[size]);

You could also do

  UniquePtr<uint8_t[]> data = MakeUnique<uint8_t[]>(size);

or

  auto data = MakeUnique<uint8_t[]>(size);

to avoid typing out the type name twice.  The former is maybe slightly more obscure, but I expect/hope, eventually, we'll get to the point where what MakeUnique does is generally well-understood such that people will see this and know |data| is a UniquePtr smart pointer that acts like an array.  Up to you whether you think that day is here yet or not (or whether it's easy enough to look up to use auto/MakeUnique anyway) -- and whether pushing the point using |auto| might itself hasten that day, if that day isn't here yet.

(I'm in the middle of writing a blog post introducing UniquePtr that'll talk about all this, but I figure I might as well point it out these sorts of things one-by-one as I see them, in the meantime.)

@@ +477,1 @@
>                  new char[LZ4::maxCompressedSize(mDatasize)]);

UniquePtr<char[]> compresseddata = MakeUnique<char[]>(LZ4::maxCompressedSize(mDatasize));

or

auto compresseddata = MakeUnique<char[]>(LZ4::maxCompressedSize(mDatasize));

perhaps.

Comment 10

[u480271]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> auto data = MakeUnique<uint8_t[]>(size);
> auto compresseddata = MakeUnique<char[]>(LZ4::maxCompressedSize(mDatasize));

I prefer the conciseness of these versions.

Comment 11

[Boris Chiou [:boris]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> Comment on attachment 8460004 [details] [diff] [review]
> Replace nsAuto(Array)Ptr with UniquePtr in LayerScope.cpp
> 
> Review of attachment 8460004 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/LayerScope.cpp
> @@ +380,5 @@
> >          if (!WebSocketHelper::GetSocketManager())
> >              return true;
> >  
> >          uint32_t size = aPacket.ByteSize();
> > +        UniquePtr<uint8_t[]> data(new uint8_t[size]);
> 
> You could also do
> 
>   UniquePtr<uint8_t[]> data = MakeUnique<uint8_t[]>(size);
> 
> or
> 
>   auto data = MakeUnique<uint8_t[]>(size);
Cool. I also prefer this one.
> 
> to avoid typing out the type name twice.  The former is maybe slightly more
> obscure, but I expect/hope, eventually, we'll get to the point where what
> MakeUnique does is generally well-understood such that people will see this
> and know |data| is a UniquePtr smart pointer that acts like an array.  Up to
> you whether you think that day is here yet or not (or whether it's easy
> enough to look up to use auto/MakeUnique anyway) -- and whether pushing the
> point using |auto| might itself hasten that day, if that day isn't here yet.
> 
> (I'm in the middle of writing a blog post introducing UniquePtr that'll talk
> about all this, but I figure I might as well point it out these sorts of
> things one-by-one as I see them, in the meantime.)
Great! I look forward to your blog about UniquePtr and want to share it with my colleagues. It's a good news.
> 
> @@ +477,1 @@
> >                  new char[LZ4::maxCompressedSize(mDatasize)]);
> 
> UniquePtr<char[]> compresseddata =
> MakeUnique<char[]>(LZ4::maxCompressedSize(mDatasize));
> 
> or
> 
> auto compresseddata = MakeUnique<char[]>(LZ4::maxCompressedSize(mDatasize));
> 
> perhaps.
Thanks for your suggestion, Jeff. I can use auto and MakeUnique in my future patch. :)