[Pixman] warnings in pixman 0.21.4

[Andrea Canciani]

On Tue, Jan 25, 2011 at 4:38 PM, Rolland Dudemaine <rolland at ghs.com> wrote:
>
>>>> - *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
>> PIXMAN_N_OPERATORS.
>
> 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 think that upon an explicit cast, a C compiler should not complain.
Otherwise the cast operator (which is obviously legitimate in C) would
warn in almost all cases in that compiler. Cast behavior is defined in the
standard and it looks like the compiler is not allowed to complain about
the value being converted not being present in the enum.

> 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.

The problem is that they should not be public and that they are special
values, not really elements of that enumeration, they are more like
wildcards.

>
> --Rolland Dudemaine
> _______________________________________________
> Pixman mailing list
> Pixman at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pixman
>