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

[Emil Velikov]

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