Switch SkAutoMalloc to SkAutoTMalloc to avoid cast

Switch SkAutoMalloc to SkAutoTMalloc to avoid cast

Make SkAutoTMalloc's interface look more like SkAutoMalloc:
- add free(), which does what you expect
- make reset() return a pointer fPtr

No public API changes (SkAutoTMalloc is in include/private)

BUG=skia:2148
Committed: https://skia.googlesource.com/skia/+/565901db954c231840750ea955ed31b820b9ade8

[scroggo, 2015-12-10 15:49:57]

The CQ bit was checked by scroggo@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-10 15:50:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516833003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516833003/1

[commit-bot: I haz the power, 2015-12-10 15:51:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-10 15:51:21]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[scroggo, 2015-12-10 15:53:33]

scroggo@google.com changed reviewers:
+ mtklein@google.com

[scroggo, 2015-12-10 15:53:34]

https://codereview.chromium.org/1516833003/diff/1/src/images/SkImageDecoder_l...
File src/images/SkImageDecoder_libjpeg.cpp (right):

https://codereview.chromium.org/1516833003/diff/1/src/images/SkImageDecoder_l...
src/images/SkImageDecoder_libjpeg.cpp:969: oneRow.reset(width * 3);
I could change SkAutoTMalloc::reset to return fPtr, like SkAutoMalloc,
SkAutoSMalloc, and SkAutoSTMalloc, so I do not need to split this into two
lines.

[mtklein, 2015-12-10 16:00:03]

lgtm

I trust fixing the typos won't affect things too much. :)

https://codereview.chromium.org/1516833003/diff/1/src/images/SkImageDecoder_l...
File src/images/SkImageDecoder_libjpeg.cpp (right):

https://codereview.chromium.org/1516833003/diff/1/src/images/SkImageDecoder_l...
src/images/SkImageDecoder_libjpeg.cpp:969: oneRow.reset(width * 3);
On 2015/12/10 at 15:53:34, scroggo wrote:
> I could change SkAutoTMalloc::reset to return fPtr, like SkAutoMalloc,
SkAutoSMalloc, and SkAutoSTMalloc, so I do not need to split this into two
lines.

Sure.  Seems fine either way.

https://codereview.chromium.org/1516833003/diff/1/tests/Writer32Test.cpp
File tests/Writer32Test.cpp (right):

https://codereview.chromium.org/1516833003/diff/1/tests/Writer32Test.cpp#newc...
tests/Writer32Test.cpp:152: uint32_t* const ptr = originalData.get();
It's fairly rare that we make const pointers.  This does seem fine, but an odd
CL to add it in?

[reed1, 2015-12-10 16:04:11]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2015-12-10 16:04:12]

https://codereview.chromium.org/1516833003/diff/1/include/private/SkTemplates.h
File include/private/SkTemplates.h (right):

https://codereview.chromium.org/1516833003/diff/1/include/private/SkTemplates...
include/private/SkTemplates.h:301: void free() {
seemes like a fine method/name. Does SkAutoMalloc have the same one? Nice to
keep these two classes looking parallel.

[scroggo, 2015-12-10 16:25:06]

The CQ bit was checked by scroggo@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-10 16:25:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516833003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516833003/20001

[scroggo, 2015-12-10 16:26:46]

https://codereview.chromium.org/1516833003/diff/1/include/private/SkTemplates.h
File include/private/SkTemplates.h (right):

https://codereview.chromium.org/1516833003/diff/1/include/private/SkTemplates...
include/private/SkTemplates.h:301: void free() {
On 2015/12/10 16:04:12, reed1 wrote:
> seemes like a fine method/name. Does SkAutoMalloc have the same one? Nice to
> keep these two classes looking parallel.

Yes. That supports my desire to make reset() return fPtr.

https://codereview.chromium.org/1516833003/diff/1/tests/Writer32Test.cpp
File tests/Writer32Test.cpp (right):

https://codereview.chromium.org/1516833003/diff/1/tests/Writer32Test.cpp#newc...
tests/Writer32Test.cpp:152: uint32_t* const ptr = originalData.get();
On 2015/12/10 16:00:03, mtklein wrote:
> It's fairly rare that we make const pointers.  This does seem fine, but an odd
> CL to add it in?

Oh, right; I meant to just use SkAutoTMalloc::operator[], but then forgot. D'oh.
Updated in next patch set.

https://codereview.chromium.org/1516833003/diff/20001/samplecode/SampleFilter...
File samplecode/SampleFilterFuzz.cpp (right):

https://codereview.chromium.org/1516833003/diff/20001/samplecode/SampleFilter...
samplecode/SampleFilterFuzz.cpp:218: SkAutoTMalloc<uint8_t> lutMemory(size);
This caused the build errors in patch set 1...

[commit-bot: I haz the power, 2015-12-10 16:29:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-10 16:29:37]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[scroggo, 2015-12-10 16:35:38]

The CQ bit was checked by scroggo@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-10 16:35:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516833003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516833003/40001

[scroggo, 2015-12-10 16:37:27]

https://codereview.chromium.org/1516833003/diff/40001/tests/Writer32Test.cpp
File tests/Writer32Test.cpp (right):

https://codereview.chromium.org/1516833003/diff/40001/tests/Writer32Test.cpp#...
tests/Writer32Test.cpp:153: originalData[(int) i] = rand.nextU();
I traded one cast for a different one here. SkAutoTMalloc's constructor takes a
size_t, and I want to iterate from 0 to that size_t, but
SkAutoTMalloc::operator[] takes an int.

[mtklein, 2015-12-10 16:38:46]

> SkAutoTMalloc's constructor takes a size_t, but SkAutoTMalloc::operator[]
takes an int.

That's pretty weird man.

[scroggo, 2015-12-10 16:50:03]

The CQ bit was checked by scroggo@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-10 16:50:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516833003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516833003/60001

[commit-bot: I haz the power, 2015-12-10 16:51:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-10 16:51:04]

Dry run: Try jobs failed on following builders:
  Build-Mac10.9-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.9-Clang-...)

[scroggo, 2015-12-10 16:51:05]

On 2015/12/10 16:38:46, mtklein wrote:
> > SkAutoTMalloc's constructor takes a size_t, but SkAutoTMalloc::operator[]
> takes an int.
> 
> That's pretty weird man.

Agreed. Fixed in patch set 4. I was worried this would snowball, and there were
a lot of places I would have to change as a result. Running a try to test...

[scroggo, 2015-12-10 17:05:39]

On 2015/12/10 16:51:05, scroggo wrote:
> On 2015/12/10 16:38:46, mtklein wrote:
> > > SkAutoTMalloc's constructor takes a size_t, but SkAutoTMalloc::operator[]
> > takes an int.
> > 
> > That's pretty weird man.
> 
> Agreed. Fixed in patch set 4. I was worried this would snowball, and there
were
> a lot of places I would have to change as a result. Running a try to test...

I don't have a strong preference between fixing these (patch set 4) and leaving
that out (patch set 3). (Slight towards 3, which is easier for me, but let me
know your preference.)

[mtklein, 2015-12-10 17:30:50]

On 2015/12/10 at 17:05:39, scroggo wrote:
> On 2015/12/10 16:51:05, scroggo wrote:
> > On 2015/12/10 16:38:46, mtklein wrote:
> > > > SkAutoTMalloc's constructor takes a size_t, but
SkAutoTMalloc::operator[]
> > > takes an int.
> > > 
> > > That's pretty weird man.
> > 
> > Agreed. Fixed in patch set 4. I was worried this would snowball, and there
were
> > a lot of places I would have to change as a result. Running a try to test...
> 
> I don't have a strong preference between fixing these (patch set 4) and
leaving that out (patch set 3). (Slight towards 3, which is easier for me, but
let me know your preference.)

I'd also stick to the focused PS3 now that we've seen PS4.

[scroggo, 2015-12-10 17:38:28]

Description was changed from

==========
Switch SkAutoMalloc to SkAutoTMalloc to avoid cast

BUG=skia:2148
==========

to

==========
Switch SkAutoMalloc to SkAutoTMalloc to avoid cast

Make SkAutoTMalloc's interface look more like SkAutoMalloc:
- add free(), which does what you expect
- make reset() return a pointer fPtr

BUG=skia:2148
==========

[scroggo, 2015-12-10 17:39:23]

Description was changed from

==========
Switch SkAutoMalloc to SkAutoTMalloc to avoid cast

Make SkAutoTMalloc's interface look more like SkAutoMalloc:
- add free(), which does what you expect
- make reset() return a pointer fPtr

BUG=skia:2148
==========

to

==========
Switch SkAutoMalloc to SkAutoTMalloc to avoid cast

Make SkAutoTMalloc's interface look more like SkAutoMalloc:
- add free(), which does what you expect
- make reset() return a pointer fPtr

No public API changes (SkAutoTMalloc is in include/private)

BUG=skia:2148
==========

[scroggo, 2015-12-10 17:39:58]

On 2015/12/10 17:30:50, mtklein wrote:
> On 2015/12/10 at 17:05:39, scroggo wrote:
> > On 2015/12/10 16:51:05, scroggo wrote:
> > > On 2015/12/10 16:38:46, mtklein wrote:
> > > > > SkAutoTMalloc's constructor takes a size_t, but
> SkAutoTMalloc::operator[]
> > > > takes an int.
> > > > 
> > > > That's pretty weird man.
> > > 
> > > Agreed. Fixed in patch set 4. I was worried this would snowball, and there
> were
> > > a lot of places I would have to change as a result. Running a try to
test...
> > 
> > I don't have a strong preference between fixing these (patch set 4) and
> leaving that out (patch set 3). (Slight towards 3, which is easier for me, but
> let me know your preference.)
> 
> I'd also stick to the focused PS3 now that we've seen PS4.

Done. reed@, there are no public API changes, but given that you had comments
earlier, do you want to look it over again?

[reed1, 2015-12-10 18:30:33]

lgtm

[scroggo, 2015-12-10 18:31:08]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-12-10 18:31:08]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1516833003/#ps80001
(title: "Go back to patch set 3")

[commit-bot: I haz the power, 2015-12-10 18:31:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516833003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516833003/80001

[commit-bot: I haz the power, 2015-12-10 18:44:16]

Description was changed from

==========
Switch SkAutoMalloc to SkAutoTMalloc to avoid cast

Make SkAutoTMalloc's interface look more like SkAutoMalloc:
- add free(), which does what you expect
- make reset() return a pointer fPtr

No public API changes (SkAutoTMalloc is in include/private)

BUG=skia:2148
==========

to

==========
Switch SkAutoMalloc to SkAutoTMalloc to avoid cast

Make SkAutoTMalloc's interface look more like SkAutoMalloc:
- add free(), which does what you expect
- make reset() return a pointer fPtr

No public API changes (SkAutoTMalloc is in include/private)

BUG=skia:2148

Committed:
https://skia.googlesource.com/skia/+/565901db954c231840750ea955ed31b820b9ade8
==========

[commit-bot: I haz the power, 2015-12-10 18:44:17]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/565901db954c231840750ea955ed31b820b9ade8