[Pixman] warnings in pixman 0.21.4
rolland at ghs.com
Tue Jan 25 07:38:15 PST 2011
>>> - *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.
Ultimately, the function should probably be splitted in two so as not to
rely on the compiler to optimize away half of the code in each case.
>>> - *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
I'm not sure what a "clever" compiled would do with a value that doesn't
belong to the enum is casted to the enum, but I would expect it to
complain that the value is not defined in the enum, resulting in a
different but nonetheless replacement warning.
I'm not sure what the issue is with adding those dummy ones to the enum.
If your concern is that you cannot count the number of elements in the
enum, then maybe one entry should be added to define a _MAX value.
More information about the Pixman