[Pixman] warnings in pixman 0.21.4

[Siarhei Siamashka]

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 ;)

> - *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?

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.

-- 
Best regards,
Siarhei Siamashka