Closed Bug 1218823 Opened 4 years ago Closed 4 years ago

use UniquePtr<> in preference to delete[] in image/

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(1 file)

No description provided.
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 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+
(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)
(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)
https://hg.mozilla.org/mozilla-central/rev/912688ff102a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.