[Pixman] [RFC 1/2] Remove seemingly unneeded comparison(s)
ppaalanen at gmail.com
Fri Apr 29 10:35:01 UTC 2016
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 ?
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 811 bytes
Desc: OpenPGP digital signature
More information about the Pixman