|
|
|
Created:
14 years, 6 months ago by scr Modified:
14 years, 6 months ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk Visibility:
Public. |
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=
Patch Set 1 #Patch Set 2 : Adjusted to taking the point pointer and walking each component of the point in exceeds_dist #Patch Set 3 : deref when comparing. #
Total comments: 6
Total messages: 5
|
scr
|
14 years, 6 months ago (2011年06月22日 00:45:04 UTC) #1 |
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.
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.
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.
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.