Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2006143009:
Initialize the result in failure conditions of ChopMonoAtY. (Closed)
|
Created:
4 years, 7 months ago by Martin Barbella Modified:
4 years, 6 months ago CC:
reviews_skia.org Base URL:
https://chromium.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionCheck results from calls to SkCubicClipper::ChopMonoAtY.
BUG=613918
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2006143009
Committed: https://skia.googlesource.com/skia/+/276e63361c73fed6c6528b322400ece81fd1d067
Patch Set 1 #Patch Set 2 : Check return values at call-sites rather than ensuring initialization #Messages
Total messages: 17 (7 generated)
Description was changed from ========== Initialize the result in failure conditions of ChopMonoAtY. BUG=613918 ========== to ========== Initialize the result in failure conditions of ChopMonoAtY. BUG=613918 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2006143009 ==========
mbarbella@chromium.org changed reviewers: + bsalomon@google.com
PTAL when you have a chance.
bsalomon@google.com changed reviewers: + reed@google.com
+reed@. My guess is that the intent is to not modify the out param if the function returns false.
FWIW, in the NEWTON_RAPHSON case above it seems like it's always initialized. It seems like a better practice to me to ensure it's initialized rather than have to worry about this at every call-site.
reed@google.com changed reviewers: + caryclark@google.com
Initializing t to something when we're going to return false just hides a larger
bug, so I'd prefer we fix the larger bug (since no value of t will give a
meaningful answer to the caller).
The calling code is here I think:
static void tangent_cubic(const SkPoint pts[], SkScalar x, SkScalar y,
SkTDArray<SkVector>* tangents) {
if (!between(pts[0].fY, y, pts[1].fY) && !between(pts[1].fY, y, pts[2].fY)
&& !between(pts[2].fY, y, pts[3].fY)) {
return;
}
if (!between(pts[0].fX, x, pts[1].fX) && !between(pts[1].fX, x, pts[2].fX)
&& !between(pts[2].fX, x, pts[3].fX)) {
return;
}
SkPoint dst[10];
int n = SkChopCubicAtYExtrema(pts, dst);
for (int i = 0; i <= n; ++i) {
SkPoint* c = &dst[i * 3];
SkScalar t;
SkAssertResult(SkCubicClipper::ChopMonoAtY(c, y, &t));
SkScalar xt = eval_cubic_pts(c[0].fX, c[1].fX, c[2].fX, c[3].fX, t);
if (!SkScalarNearlyEqual(x, xt)) {
continue;
}
SkVector tangent;
SkEvalCubicAt(c, t, nullptr, &tangent, nullptr);
tangents->push(tangent);
}
}
I think what is happening is that ChopMonoAtY() is returning false (hence t is
uninitialized) but the code does not check for that (only in Debug does it
assert). It seems like the proper fix is to check the return result and handle
false here.
+cary who worked on this area I think.
I think we should also add more SK_WARN_UNUSED_RESULT annotations to routines
like these. I will work on that now.
On 2016/05/26 18:00:22, reed1 wrote:
> Initializing t to something when we're going to return false just hides a
larger
> bug, so I'd prefer we fix the larger bug (since no value of t will give a
> meaningful answer to the caller).
>
> The calling code is here I think:
>
> static void tangent_cubic(const SkPoint pts[], SkScalar x, SkScalar y,
> SkTDArray<SkVector>* tangents) {
> if (!between(pts[0].fY, y, pts[1].fY) && !between(pts[1].fY, y, pts[2].fY)
> && !between(pts[2].fY, y, pts[3].fY)) {
> return;
> }
> if (!between(pts[0].fX, x, pts[1].fX) && !between(pts[1].fX, x, pts[2].fX)
> && !between(pts[2].fX, x, pts[3].fX)) {
> return;
> }
> SkPoint dst[10];
> int n = SkChopCubicAtYExtrema(pts, dst);
> for (int i = 0; i <= n; ++i) {
> SkPoint* c = &dst[i * 3];
> SkScalar t;
> SkAssertResult(SkCubicClipper::ChopMonoAtY(c, y, &t));
> SkScalar xt = eval_cubic_pts(c[0].fX, c[1].fX, c[2].fX, c[3].fX, t);
> if (!SkScalarNearlyEqual(x, xt)) {
> continue;
> }
> SkVector tangent;
> SkEvalCubicAt(c, t, nullptr, &tangent, nullptr);
> tangents->push(tangent);
> }
> }
>
> I think what is happening is that ChopMonoAtY() is returning false (hence t is
> uninitialized) but the code does not check for that (only in Debug does it
> assert). It seems like the proper fix is to check the return result and handle
> false here.
>
> +cary who worked on this area I think.
>
> I think we should also add more SK_WARN_UNUSED_RESULT annotations to routines
> like these. I will work on that now.
Having SK_WARN_UNUSED_RESULT on these sounds even better, thanks for taking a
look.
On 2016/05/26 18:02:43, Martin Barbella wrote:
> On 2016/05/26 18:00:22, reed1 wrote:
> > Initializing t to something when we're going to return false just hides a
> larger
> > bug, so I'd prefer we fix the larger bug (since no value of t will give a
> > meaningful answer to the caller).
> >
> > The calling code is here I think:
> >
> > static void tangent_cubic(const SkPoint pts[], SkScalar x, SkScalar y,
> > SkTDArray<SkVector>* tangents) {
> > if (!between(pts[0].fY, y, pts[1].fY) && !between(pts[1].fY, y,
pts[2].fY)
> > && !between(pts[2].fY, y, pts[3].fY)) {
> > return;
> > }
> > if (!between(pts[0].fX, x, pts[1].fX) && !between(pts[1].fX, x,
pts[2].fX)
> > && !between(pts[2].fX, x, pts[3].fX)) {
> > return;
> > }
> > SkPoint dst[10];
> > int n = SkChopCubicAtYExtrema(pts, dst);
> > for (int i = 0; i <= n; ++i) {
> > SkPoint* c = &dst[i * 3];
> > SkScalar t;
> > SkAssertResult(SkCubicClipper::ChopMonoAtY(c, y, &t));
> > SkScalar xt = eval_cubic_pts(c[0].fX, c[1].fX, c[2].fX, c[3].fX, t);
> > if (!SkScalarNearlyEqual(x, xt)) {
> > continue;
> > }
> > SkVector tangent;
> > SkEvalCubicAt(c, t, nullptr, &tangent, nullptr);
> > tangents->push(tangent);
> > }
> > }
> >
> > I think what is happening is that ChopMonoAtY() is returning false (hence t
is
> > uninitialized) but the code does not check for that (only in Debug does it
> > assert). It seems like the proper fix is to check the return result and
handle
> > false here.
> >
> > +cary who worked on this area I think.
> >
> > I think we should also add more SK_WARN_UNUSED_RESULT annotations to
routines
> > like these. I will work on that now.
>
> Having SK_WARN_UNUSED_RESULT on these sounds even better, thanks for taking a
> look.
I wouldn't mind trying to fix the caller later today, but if anyone more
familiar with the area wants to steal this bug, feel free.
Description was changed from ========== Initialize the result in failure conditions of ChopMonoAtY. BUG=613918 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2006143009 ========== to ========== Check results from calls to SkCubicClipper::ChopMonoAtY. BUG=613918 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2006143009 ==========
Not sure what the right behavior in these cases should be, so sorry if this is way off-base. Any input would be appreciated.
lgtm
The CQ bit was checked by mbarbella@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006143009/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006143009/20001
Message was sent while issue was closed.
Description was changed from ========== Check results from calls to SkCubicClipper::ChopMonoAtY. BUG=613918 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2006143009 ========== to ========== Check results from calls to SkCubicClipper::ChopMonoAtY. BUG=613918 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2006143009 Committed: https://skia.googlesource.com/skia/+/276e63361c73fed6c6528b322400ece81fd1d067 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/276e63361c73fed6c6528b322400ece81fd1d067 |
