[Pixman] warnings in pixman 0.21.4

[Andrea Canciani]

On Tue, Jan 25, 2011 at 3:23 PM, Siarhei Siamashka
<siarhei.siamashka at gmail.com> wrote:
> On Monday 24 January 2011 11:51:39 Rolland Dudemaine wrote:
>> Hi,
>>
>> Granted, the patch was somewhat too big and contained many things.
>>
>> Attached the patch splitted in five parts :
>> - *fence_prototype.diff resolves the incorrect fence_malloc prototype.
>> It also correctly protects onglobal_msg and on_alarm behind
>> HAVE_SIGACTION and HAVE_ALARM, since it's otherwise useless and
>> unreferenced.  If you still want to split these further into separate
>> patches, feel free to do so, this is trivial enough to do.
>
> Pushed this fix (without 'on_alarm' so far) to pixman git master.
>
>> - *max_vx.diff corrects the initialization of max_vx. Contrary to what
>> you mention, the variable is declared in a local block, so the scope of
>> the variable is restricted to that block, regardless of whether it is
>> part of a macro or not. So the compiler is correct saying the value may
>> be used before being initialized. Also, INT32_MAX is a safe value,
>> because pixman-compiler.h defines it. INT32_MAX is the right value to
>> use since max_vx is a clamping value passed on to a callee. 0 would just
>> clamp down to 0, which would be plainly incorrect.
>
> Pushed this one too. Even though I think that your analysis is wrong, I don't
> want to waste time arguing about the choice of the constant ;)

I agree with Siarhei, max_vx is used in scanline_func if and only if
the repeat mode is NORMAL, so it should not actually matter.
Anyway, INT32_MAX is obviously a safe value so your patch is ok for me.
Since any value is fine in that initialization, the problem with pixman_fixed_t
becoming a different type in future is moot as well. I think that initializing
to 0 would be ok as well, but I doubt that it's an optimization worth the
additional readability cost.

>
>> - *return_values.diff defines return values where missing. I added it as
>> a general return value, you may want to move that inside the switch
>> statement right above, this is really a coding style issue.
>>
>> - *set_but_not_used.diff removes useless variable declarations. This can
>> only result in more efficient code, as these variables where sometimes
>> assigned, but their values were never used.
>>
>> - *correct_dimmy_PIXMAN_types.diff is the most complex part of the
>> patches. My compiler was producing a number of warnings due to incorrect
>> uses of int instead of the correct corresponding pixman_*_t type. Also,
>> it was complaining (rightfully) that some values were passed as
>> pixman_format_code_t that were not defined in that enum. The best
>> resolution I could come up with was to logically move PIXMAN_null,
>> PIXMAN_any and the related dummy pixmap types to the enum, so that it is
>> possible to use them as argument where a pixman_format_code_t argument
>> is required. (Note : the attached patch contains one more correction for
>> blitters-test.c that I did not correct previously)
>
> The biggest complaint against 'correct_dimmy_PIXMAN_types.diff' is that it
> moves the definitions of PIXMAN_solid, PIXMAN_pixbuf, PIXMAN_rpixbuf,
> PIXMAN_unknown, PIXMAN_any, PIXMAN_OP_any to the public header 'pixman.h'.
> And these are intended for internal use only and not supposed to be part of
> external API. Are there any good alternative fixes for the problem?

We can add a cast to pixman_op_t or pixman_format_code_t in the #defines.
Another solution (which I don't like) would be to protect the new enum values
with an "#ifdef PIXMAN_USE_INTERNAL_API", just like in pixman.h for
PIXMAN_N_OPERATORS.

>
> PIXMAN_null is a bit different, because it indeed should by returned from
> 'pixman_image_get_format' function instead of 0.
>
> Another problem is that it is an extra burden on me (or any other committer)
> to come up with commit message texts for these warnings cleanup patches,
> which are even not bugfixes. It would have been much easier to deal with
> the patches generated by 'git format-patch'
>
> I have put some of the other your changes (but not all yet) into this branch:
> http://cgit.freedesktop.org/~siamashka/pixman/log/?h=warnings-fixes
> If everybody is ok with these changes and commit messages, I'll push them
> to pixman git repository shortly. Then we can look at what is still remaining.

The branch looks good to me.

Andrea

>
> --
> Best regards,
> Siarhei Siamashka
> _______________________________________________
> Pixman mailing list
> Pixman at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pixman
>