Closed
Bug 1216644
Opened 9 years ago
Closed 9 years ago
get rid of nsAutoArrayPtr usage in gfx/
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Attachments
(3 files, 2 obsolete files)
6.07 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
16.21 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
11.57 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
This patch handles all the cases where we just want to swap out nsAutoArrayPtr for UniquePtr and don't require code changes other than updating uses that implicitly converted to T* to follow UniquePtr's conventions. I chose &x[0] instead of x.get() for such uses as &x[0] is slightly shorter and feels more idiomatic.
Attachment #8676359 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 2•9 years ago
|
||
This patch could have been in the last patch, but it felt like modifying ReadCMAPTableFormat14 to accept a UniquePtr<> made the code clearer. This change was therefore separated into its own patch for easier review.
Attachment #8676360 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 3•9 years ago
|
||
Changing mRecycledBuffers store UniquePtrs instead of nsAutoArrayPtrs opens up the possibility of a reasonable facsimile of ownership in function signatures. To do that, we have to make PlanarYCbCrImage::AllocateBuffer return UniquePtr, which necessitates changing a few overloads. The SharedPlanarYCbCrImage change for this is a little dodgy, but does make implicit assumptions made prior to this patch a tad more explicit.
Attachment #8676361 -
Flags: review?(jmuizelaar)
Updated•9 years ago
|
Attachment #8676360 -
Flags: review?(jmuizelaar) → review+
Comment 4•9 years ago
|
||
Comment on attachment 8676359 [details] [diff] [review] part 1 - simple s/nsAutoArrayPtr/UniquePtr/ changes in gfx/ Review of attachment 8676359 [details] [diff] [review]: ----------------------------------------------------------------- I personally like x.get() better. &x[0] reads to me as get the address of the first element and not get the address of the array, but I'm willing to resign to consistency if needed. It seems like this code would be all around better if we had something like dyn_array and array_view from CppCoreGuidelines.
Attachment #8676359 -
Flags: review?(jmuizelaar) → review+
(.get() is consistent with the other smart pointer types for getting the underlying pointer, too)
Comment 6•9 years ago
|
||
Comment on attachment 8676361 [details] [diff] [review] part 3 - make BufferRecycleBin store UniquePtrs Review of attachment 8676361 [details] [diff] [review]: ----------------------------------------------------------------- I'm not comfortable with the change in SharedPlanarYCbCrImage. It makes the ownership explicit when that's not necessarily true. I'm going to try and dig into an alternative that will keep us from having to lie.
Attachment #8676361 -
Flags: review?(jmuizelaar) → review-
Comment 7•9 years ago
|
||
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #5) > (.get() is consistent with the other smart pointer types for getting the > underlying pointer, too) Yeah, the more I think about it, the more I prefer .get(). The brevity argument for &x[0] doesn't appeal to me at all. Anytime, you write this you should be thinking "Am I sure I'm not introducing a use-after-free here" and I think .get() communicates that a little more explicitly than &x[0]
Comment 8•9 years ago
|
||
Comment on attachment 8676359 [details] [diff] [review] part 1 - simple s/nsAutoArrayPtr/UniquePtr/ changes in gfx/ Review of attachment 8676359 [details] [diff] [review]: ----------------------------------------------------------------- I personally like x.get() better. &x[0] reads to me as get the address of the first element and not get the address of the array, but I'm willing to resign to consistency if needed. It seems like this code would be all around better if we had something like dyn_array and array_view from CppCoreGuidelines.
Attachment #8676359 -
Flags: review+
Comment 9•9 years ago
|
||
Bug 127080 will let us avoid the SharedPlanarYCbCrImage hack
Assignee | ||
Comment 10•9 years ago
|
||
This patch handles all the cases where we just want to swap out nsAutoArrayPtr for UniquePtr and don't require code changes other than updating uses that implicitly converted to T* to follow UniquePtr's conventions. Changed to use MakeUniqueFallible and x.get() instead of &x[0].
Attachment #8686719 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•9 years ago
|
Attachment #8676359 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
Changing mRecycledBuffers to store UniquePtrs instead of nsAutoArrayPtrs opens up the possibility of a reasonable facsimile of ownership in function signatures. Rebased etc.
Attachment #8686720 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•9 years ago
|
Attachment #8676361 -
Attachment is obsolete: true
Comment 12•9 years ago
|
||
Comment on attachment 8686719 [details] [diff] [review] part 1 - simple s/nsAutoArrayPtr/UniquePtr/ changes in gfx/ Review of attachment 8686719 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/gl/GLBlitHelper.cpp @@ +370,5 @@ > break; > } > > + UniquePtr<char[]> buffer(new char[length]); > + mGL->fGetProgramInfoLog(program, length, nullptr, &buffer[0]); buffer.get()
Attachment #8686719 -
Flags: review?(jmuizelaar) → review+
Comment 13•9 years ago
|
||
Comment on attachment 8686720 [details] [diff] [review] part 3 - make BufferRecycleBin store UniquePtrs Review of attachment 8686720 [details] [diff] [review]: ----------------------------------------------------------------- This is a nice patch now. ::: gfx/layers/ImageContainer.h @@ +734,5 @@ > > /** > * Return a buffer to store image data in. > * The default implementation returns memory that can > + * be freed with delete[] I think we can just drop "The default implementation returns memory that can be freed wit delete[]" comment as the UniquePtr is now responsible for freeing the memory instead of caller.
Updated•9 years ago
|
Attachment #8686720 -
Flags: review?(jmuizelaar) → review+
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/db42f398f454 https://hg.mozilla.org/integration/mozilla-inbound/rev/4ca297b17a82 https://hg.mozilla.org/integration/mozilla-inbound/rev/198d31b65082
Comment 15•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/db42f398f454 https://hg.mozilla.org/mozilla-central/rev/4ca297b17a82 https://hg.mozilla.org/mozilla-central/rev/198d31b65082
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•