[Pixman] [RFC 1/2] Remove seemingly unneeded comparison(s)

[Bill Spitzak]

If the comparison fails, the returned values are modulus fixed_16_16,
rather than clamped. I think that gives a lot less possibilities for the
calling code to recover from or simulate the results of the out-of-range
value.


On Fri, Apr 29, 2016 at 6:33 AM, Emil Velikov <emil.l.velikov at gmail.com>
wrote:

> On 29 April 2016 at 11:35, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Fri, 29 Apr 2016 10:15:37 +0100
> > Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >
> >> On 27 April 2016 at 18:46, Bill Spitzak <spitzak at gmail.com> wrote:
> >> > On Wed, Apr 27, 2016 at 2:03 AM, Pekka Paalanen <ppaalanen at gmail.com>
> wrote:
> >> >>
> >> >> On Wed, 27 Apr 2016 09:56:44 +0100
> >> >> Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >> >>
> >> >> > On 26 April 2016 at 19:12, Bill Spitzak <spitzak at gmail.com> wrote:
> >> >> > > The old code is comparing pixman_fixed_48_16_t values to
> >> >> > > pixman_fixed_16_16_t values, thus it is checking for truncation
> of
> >> >> > > overflow
> >> >> > > values.
> >> >> > >
> >> >> > Indeed it does. I'll grep more before asking silly questions ;-)
> >> >> >
> >> >> > > It would probably be better to clamp these overflowed values,
> like
> >> >> > > pixman_transform_point_31_16 is doing to clamp to the
> >> >> > > pixman_fixed_48_16
> >> >> > > result. Right now the result is an odd mix of clamping and
> modulus. A
> >> >> > > rewrite to go directly to clamped pixman_fixed_16_16 values
> would be
> >> >> > > even
> >> >> > > better.
> >> >> > >
> >> >> > Sounds like a plan. Sadly I doubt I'll get to it any time soon.
> >> >>
> >> >> Wasn't the point of the boolean return from these functions to tell
> the
> >> >> caller to drop what it is doing because it cannot be done properly?
> >> >
> >> >
> >> > Dropping a fill is a lot worse than trying to simulate it using the
> clamped
> >> > path. It will produce a wrong result if one of the edges connected to
> a
> >> > clamped point passes through the clip, but this often does not
> happen, and
> >> > the transition to the wrong result is gradual as the point moves
> outside the
> >> > clamped region.
> >> >
> >> > More importantly the caller cannot do anything with the return values
> right
> >> > now, as they are modulus MAX_16_16+1. Even the direction they are in
> is
> >> > lost.
> >> >
> >> I think that keeping the user provided memory as-is when the function
> >> does not succeed is a good idea.
> >> Afaics currently the contents get overwritten regardless of the result.
> >>
> >> This is what you guys were on about, right ? Or perhaps you're
> >> thinking about spinning v2 of the function with different
> >> signature/behaviour ?
> >
> > Hi Emil,
> >
> > I think the conclusion was that the comparisons are not useless, and
> > this patch should be dropped. You noted it yourself that this patch
> > causes a regression in the test suite.
> >
> Fully agree on both points. Just trying to understand what you and
> Bill are talking about and suggest that if one changes things, would
> be nice to avoid "feeding garbage" back to the user [on error].
>
> Thanks
> Emil
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/pixman/attachments/20160429/904dc068/attachment.html>