Closed
Bug 1218823
Opened 9 years ago
Closed 9 years ago
use UniquePtr<> in preference to delete[] in image/
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
Details
Attachments
(1 file)
11.49 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
The only remaining use of delete[] is in Downscaler.cpp; I tried removing it, but was defeated by the assumption in DownscaleInputLine that mWindow is managing a raw array-of-arrays. Some static_asserts and reinterpret_cast would enable working around that, but it seemd better to just suck it up and deal.
Attachment #8679475 -
Flags: review?(seth)
Comment 2•9 years ago
|
||
Comment on attachment 8679475 [details] [diff] [review] use UniquePtr<> in preference to delete[] in image/ Review of attachment 8679475 [details] [diff] [review]: ----------------------------------------------------------------- Much better! No worries about the Downscaler code. At some point in the future I'm hoping to rewrite it to be a bit cleaner, and at that time I'll try to eliminate the delete[] call. ::: image/encoders/jpeg/nsJPEGEncoder.cpp @@ +162,5 @@ > jpeg_write_scanlines(&cinfo, const_cast<uint8_t**>(&row), 1); > } > } else if (aInputFormat == INPUT_FORMAT_RGBA) { > + UniquePtr<uint8_t[]> rowptr = MakeUnique<uint8_t[]>(aWidth * 3); > + uint8_t* row = rowptr.get(); Any reason not to just call row.get() in the call to ConvertRGBARow() below? All things being equal, I'd prefer that. @@ +171,3 @@ > } else if (aInputFormat == INPUT_FORMAT_HOSTARGB) { > + UniquePtr<uint8_t[]> rowptr = MakeUnique<uint8_t[]>(aWidth * 3); > + uint8_t* row = rowptr.get(); Same.
Attachment #8679475 -
Flags: review?(seth) → review+
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Seth Fowler [:seth] [:s2h] from comment #2) > ::: image/encoders/jpeg/nsJPEGEncoder.cpp > @@ +162,5 @@ > > jpeg_write_scanlines(&cinfo, const_cast<uint8_t**>(&row), 1); > > } > > } else if (aInputFormat == INPUT_FORMAT_RGBA) { > > + UniquePtr<uint8_t[]> rowptr = MakeUnique<uint8_t[]>(aWidth * 3); > > + uint8_t* row = rowptr.get(); > > Any reason not to just call row.get() in the call to ConvertRGBARow() below? > All things being equal, I'd prefer that. Are you saying that I should keep |row| here and pass |rowptr.get()| to ConvertRGBARow()? Or are you saying I should eliminate the temporary entirely? The problem this temporary is addressing is that jpeg_write_scanlines takes uint8_t**, and we can't pass &row.get() to jpeg_write_scanlines. I suppose we could shuffle the temporary variable to just before the jpeg_write_scanlines call if you like?
Flags: needinfo?(seth)
Comment 4•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #3) > The problem this temporary is addressing is that jpeg_write_scanlines takes > uint8_t**, and we can't pass &row.get() to jpeg_write_scanlines. I suppose > we could shuffle the temporary variable to just before the > jpeg_write_scanlines call if you like? I see; that wasn't immediately obvious just looking at the patch in Splinter. Let's just leave the patch as-is then; I don't think we gain much unless we can actually get rid of the temporary.
Flags: needinfo?(seth)
Comment 6•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/912688ff102a
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•