Replace nsAutoPtr with UniquePtr in LayerScope.cpp

RESOLVED FIXED in mozilla34

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jwatt, Assigned: boris)

Tracking

(Blocks: 1 bug)

Trunk
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
(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.
(Assignee)

Updated

4 years ago
Assignee: jwatt → boris.chiou
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Depends on: 991227
(Assignee)

Comment 2

4 years ago
(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.
Blocks: 959112
(Assignee)

Updated

4 years ago
Summary: Stop abusing std::auto_ptr in LayerScope.cpp → Replace nsAutoPtr with UniquePtr in LayerScope.cpp
(Assignee)

Comment 3

4 years ago
(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.
(Assignee)

Comment 4

4 years ago
Created attachment 8460004 [details] [diff] [review]
Replace nsAuto(Array)Ptr with UniquePtr in LayerScope.cpp
(Assignee)

Comment 5

4 years ago
(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
(Assignee)

Updated

4 years ago
Attachment #8460004 - Flags: review?(dglastonbury)
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.
Attachment #8460004 - Flags: review?(dglastonbury) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Comment 8

4 years ago
https://hg.mozilla.org/mozilla-central/rev/56e92647d661
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

Comment 9

4 years ago
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.
(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.
(Assignee)

Comment 11

4 years ago
(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. :)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.