views: Delete arrays throughout using SkDELETE_ARRAY macro.

views: Delete arrays throughout using SkDELETE_ARRAY macro.

R=epoger@google.org

[epoger, 2012-09-20 18:56:35]

Adding Mike Reed, because I have some very basic concerns about this code BEFORE
these changes were made, and history shows that the content of this file is
quite old:
https://code.google.com/p/skia/source/list?path=/trunk/src/views/animated/SkL...

https://codereview.appspot.com/6550046/diff/4001/src/views/animated/SkListVie...
File src/views/animated/SkListView.cpp (right):

https://codereview.appspot.com/6550046/diff/4001/src/views/animated/SkListVie...
src/views/animated/SkListView.cpp:164: SkDELETE_ARRAY(fList);
I see that SkDELETE_ARRAY is defined in
https://code.google.com/p/skia/source/browse/trunk/include/core/SkPostConfig.h
...

Why change delete[] to SkDELETE_ARRAY, but not new to SkNEW?

Was this code causing a specific problem that this CL fixes?  (AFAICT,
SkDELETE_ARRAY is a macro for delete[] anyway, so the code doesn't change.)

Maybe the safest thing would be to make fList an SkAutoTArray (see
https://code.google.com/p/skia/source/browse/trunk/include/core/SkTemplates.h )
so it deletes itself automatically?

https://codereview.appspot.com/6550046/diff/4001/src/views/animated/SkListVie...
src/views/animated/SkListView.cpp:228: SkDELETE_ARRAY(fStrCache);
This doesn't look safe at all.  The constructor doesn't even allocate the
array... so what if the destructor is called before ensureStrCache() allocates
it?

AFAICT, the only place ensureStrCache() is called is in onDraw(), so does this
cause problems if the object is destroyed before onDraw() is called?

Seems like maybe another place for an automatic-type object, as above...

https://codereview.appspot.com/6550046/diff/4001/src/views/animated/SkListVie...
src/views/animated/SkListView.cpp:229: fSource->safeUnref();
I'm not excited about this line either.  The constructor sets fSource=NULL, and
the destructor dereferences it without checking for NULL.

[reed1, 2012-09-20 19:31:08]

SkListView has all of its code inside #if 0 I think, so we can't test this
change.

SkListWidget -- I agree that if we're going to call SkDELETE_ARRAY, we need to
balance that with SkNEW_ARRAY.

[tfarina1, 2012-09-21 01:01:05]

On 2012/09/20 19:31:08, reed1 wrote:
> SkListView has all of its code inside #if 0 I think, so we can't test this
> change.
> 
> SkListWidget -- I agree that if we're going to call SkDELETE_ARRAY, we need to
> balance that with SkNEW_ARRAY.

Done. PTAL.

[reed1, 2012-09-21 12:30:36]

That *looks* better, but as we don't ever build the first file, I don't
understand how we can test that it even compiles.

The 2nd file is built, but afaik we don't ever run it. Do you have any test that
exercises this, so we can tell that we haven't regressed anything?

summary: its hard to want to spend much time "fixing" or reviewing code that we
don't know how to test.

[tfarina1, 2012-09-21 12:50:02]

On Fri, Sep 21, 2012 at 9:30 AM,  <reed@google.com> wrote:
> That *looks* better, but as we don't ever build the first file, I don't
> understand how we can test that it even compiles.
>
Why bother? I can revert the changes to that file. But I don't care
too much though.

> The 2nd file is built, but afaik we don't ever run it. Do you have any
> test that exercises this, so we can tell that we haven't regressed
> anything?
>
Why do you think it needs tests? I fail to see why it needs one. Can't
you check by inspecting the diff to see if the logic is yet the same?

-- 
Thiago

[epoger, 2012-09-25 18:29:02]

This CL has been rendered moot by the following CLs, so please close it:
- https://code.google.com/p/skia/source/detail?r=5667 ("Delete SkListView.cpp,
which has been #ifdef'ed out for 4+ years")
- https://code.google.com/p/skia/source/detail?r=5668 ("Remove SkListWidget.cpp,
which has not been compiled for 18 months")

Looking at the broader picture, though, I would like to echo Mike's comments. 
In general, we should not make changes to files that are not exercised by tests.
 If the file is worth changing, it's worth adding it to the tests first.