[Pixman] warnings in pixman 0.21.4

[Siarhei Siamashka]

On Saturday 22 January 2011 21:37:18 Rolland Dudemaine wrote:
> Hi,
> 
> Attached is a diff that resolves a number of warnings I encountered when
> porting pixman over to my RTOS.
> Most examples compile cleanly now.
> I had to move the definitions of PIXMAN_null, PIXMAN_any and related so
> they become type-safe, otherwise my compiler gives a enumeration to int
> cast warning. It is cleaner that way anyway.

Hi,

Thanks for raising this issue and for a patch proposal. Making sure that the
code is free of warnings with different compilers typically improves quality.

However it might be better to split the patch into separate parts, for each 
type of problem separately. For example, elimination of dead/unused code is
obvious and safe. The other changes may (or may not) need more careful review.

More details would be also welcome. For example, what kind of compiler was used
and what do these warnings look like?

I also have been playing with clang static analyzer just yesterday, and it
also spits quite a lot of warnings too (mostly about dead/unused code):
http://people.freedesktop.org/~siamashka/files/20110122/pixman-0.21.4-clang/

Regarding some parts of your patch:

---

+++ pixman-0.21.4/pixman/pixman-fast-path.h     2011-01-22 19:53:12.000000000 
-    pixman_fixed_t max_vx = max_vx; /* suppress uninitialized variable warning 
*/
+    pixman_fixed_t max_vx = INT32_MAX; /* suppress uninitialized variable 
warning */

This indeed makes a lot of sense to fix. This code is using self assignment to
suppress the use of uninitialized variable warning, but this is apparently not
quite portable and gcc-specific. Moreover, such code is not encouraged in gcc
itself nowadays  ('-Winit-self' is a search keyword for more information).

In pixman, this variable may be passed uninitialized as an argument for inline
function, but is not used in the inline function itself. Strictly speaking,
this behaviour is very likely wrong according to C standard even though it
appears to work fine in practice with the current compilers and cpu
architectures. I think it might be possible to rearrange the code to actually
pass optional arguments to the inline function as a pointer to some structure,
where some of the fields may be uninitialized if not needed. But just
initializing 'max_vx' to something should work for now.

But INT32_MAX is not a good choice for initializing this variable, I think 0
would be better. In the case if the compiler can't see that this value is not
actually used and optimize it out, arbitrary magic constants are handled less
efficient on RISC processors because not every constant can be used as an
immediate operand. For example, compiling the following functions for MIPS:

int f1(void)
{
    return 2147483647;
}

int f2(void)
{
    return 0;
}

Results in the following code:

00000000 <f1>:
   0:   3c027fff        lui     v0,0x7fff
   4:   03e00008        jr      ra
   8:   3442ffff        ori     v0,v0,0xffff

0000000c <f2>:
   c:   03e00008        jr      ra
  10:   00001021        move    v0,zero

Using zero as an integer constant value is clearly liked better by MIPS
processors.

Anyway, I have some more changes to this code coming and will handle this issue
too. Especially considering that it is also flagged by clang static analyzer.
Thanks.

---

Regarding another part of the patch:

 void *
-fence_malloc (uint32_t len)
+fence_malloc (int64_t len)

Ugh, that is a clear bug and the fix looks correct. I think we should push it
to pixman git.

---

I still did not look at the other parts of the patch closely, and the other
people may also provide some comments.


-- 
Best regards,
Siarhei Siamashka