Coverity Fixlet - ARRAY_VS_SINGLETON

Coverity Fixlet - ARRAY_VS_SINGLETON

Address http://chromecoverity.mtv.corp.google.com:5467/cov.cgi?cid=38

Made this function more readable by passing in the points, and doing the wackiness of casting and walking each component of the point within the static mechod

CID=38
BUG=
TEST=

[Steve VanDeBogart, 2011-06-22 20:08:00]

http://codereview.appspot.com/4627052/diff/5001/src/core/SkPath.cpp
File src/core/SkPath.cpp (left):

http://codereview.appspot.com/4627052/diff/5001/src/core/SkPath.cpp#oldcode1212
src/core/SkPath.cpp:1212: if (--subLevel >= 0 && exceeds_dist(&pts[0].fX,
&pts[1].fX, dist, 4)) {
4 doesn't make sense to me - it seems like this causes random memory to be
compared.

http://codereview.appspot.com/4627052/diff/5001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/4627052/diff/5001/src/core/SkPath.cpp#newcode1197
src/core/SkPath.cpp:1197: static bool exceeds_dist(const SkPoint* points,
SkScalar dist, int count) {
I do like this interface a lot better.

http://codereview.appspot.com/4627052/diff/5001/src/core/SkPath.cpp#newcode1201
src/core/SkPath.cpp:1201: for(const SkScalar *p = &points[0].fX, *q =
&points[1].fX;
I don't think this will actually fix the coverity warning.

[reed1, 2011-06-22 20:54:05]

1. Does this CL fix the covertly warning?
2. Have we verified that our automated tests exercise these code paths?
3. Do we understand the '4' parameter?

lets hold off on submitting this until we've all had more time to understand
what is going on.

[scr, 2011-06-22 21:08:06]

http://codereview.appspot.com/4627052/diff/5001/src/core/SkPath.cpp
File src/core/SkPath.cpp (left):

http://codereview.appspot.com/4627052/diff/5001/src/core/SkPath.cpp#oldcode1212
src/core/SkPath.cpp:1212: if (--subLevel >= 0 && exceeds_dist(&pts[0].fX,
&pts[1].fX, dist, 4)) {
This didn't make sense to me either, but it was in the previous code.  Mike - as
author, can you explain the intention of passing 4?

On 2011/06/22 20:08:00, Steve VanDeBogart wrote:
> 4 doesn't make sense to me - it seems like this causes random memory to be
> compared.

http://codereview.appspot.com/4627052/diff/5001/src/core/SkPath.cpp#oldcode1225
src/core/SkPath.cpp:1225: if (--subLevel >= 0 && exceeds_dist(&pts[0].fX,
&pts[1].fX, dist, 6)) {
Mike, this alsoi looks like it goes off the end of the array as it passes 6 as
the length, whereas it only takes 4 points.

http://codereview.appspot.com/4627052/diff/5001/src/core/SkPath.cpp
File src/core/SkPath.cpp (right):

http://codereview.appspot.com/4627052/diff/5001/src/core/SkPath.cpp#newcode1201
src/core/SkPath.cpp:1201: for(const SkScalar *p = &points[0].fX, *q =
&points[1].fX;
Previously the reference to the object was passed to an object[], now it's an
object*.  There may be a chance that Coverity flags this as well, but without a
trybot, it's tough to tell.

Would you have any objection to writing this without the cute trick to walk x's
then y's then x's, etc?  Could we walk points and check both p->fX and p->fY,
e.g.?

On 2011/06/22 20:08:00, Steve VanDeBogart wrote:
> I don't think this will actually fix the coverity warning.

[reed1, 2011-06-28 11:58:46]

The 4 means perform 4 measurements, which is correct for quads. The bug is that
I had count *= 2 before the loop. I have fixed that.

Then I grepped android and chrome, and found there are no callers for
subdivide(). As there are no callers inside Skia either, and no tests, I have
removed the function entirely.