Code quality

Code quality

[Pavel Roskin]

Hello!

It seems to me that the code quality has decreased in the last weeks.
In the same time, we have a growing number of compiler warnings.  It
looks like there is a relationship between the two.

I'll appreciate if everybody who recently contributed to GRUB looks at
the remaining warnings and fixes at least some of them.  I don't mean
hiding the warnings, e.g. adding casts where using different types would
be more appropriate or initializing variables gcc claims to be
uninitialized without making sure that gcc is indeed wrong.  But if
there is a good fix, it should be applied.

That's the warnings for the default configuration when compiled on
x86_64:

kern/i386/pc/init.c: In function 'grub_machine_fini':
kern/i386/pc/init.c:226: warning: implicit declaration of function 'grub_stop_floppy'
loader/i386/linux.c: In function 'hook':
loader/i386/linux.c:234: warning: cast to pointer from integer of different size
loader/i386/linux.c: In function 'grub_linux_setup_video':
loader/i386/linux.c:303: warning: assignment makes integer from pointer without a cast
loader/i386/pc/multiboot2.c: In function 'grub_mb2_arch_boot':
loader/i386/pc/multiboot2.c:76: warning: implicit declaration of function 'grub_multiboot2_real_boot'
loader/multiboot_loader.c: In function 'grub_cmd_multiboot_loader':
loader/multiboot_loader.c:159: warning: 'return' with no value, in function returning non-void
loader/i386/multiboot.c: In function 'grub_multiboot':
loader/i386/multiboot.c:324: warning: assignment makes integer from pointer without a cast
disk/ata.c: In function 'grub_ata_initialize':
disk/ata.c:471: warning: passing argument 1 of 'grub_pci_iterate' from incompatible pointer type
bus/usb/usbtrans.c: In function 'grub_usb_control_msg':
bus/usb/usbtrans.c:94: warning: comparison between signed and unsigned
bus/usb/usbtrans.c:94: warning: signed and unsigned type in conditional expression
commands/lspci.c: In function 'grub_cmd_lspci':
commands/lspci.c:153: warning: passing argument 1 of 'grub_pci_iterate' from incompatible pointer type
bus/usb/ohci.c: In function 'grub_ohci_inithw':
bus/usb/ohci.c:205: warning: passing argument 1 of 'grub_pci_iterate' from incompatible pointer type
bus/usb/ohci.c: In function 'grub_ohci_transaction':
bus/usb/ohci.c:235: warning: format '%02x' expects type 'unsigned int', but argument 5 has type 'grub_ohci_td_t'
bus/usb/ohci.c: In function 'grub_ohci_transfer':
bus/usb/ohci.c:297: warning: format '%08x' expects type 'unsigned int', but argument 5 has type 'grub_ohci_td_t'
bus/usb/uhci.c: In function 'grub_uhci_inithw':
bus/usb/uhci.c:305: warning: passing argument 1 of 'grub_pci_iterate' from incompatible pointer type
fs/i386/pc/pxe.c:259: warning: initialization from incompatible pointer type
disk/raid6_recover.c: In function 'grub_raid6_recover':
disk/raid6_recover.c:95: warning: 'err[0]' may be used uninitialized in this function
disk/raid6_recover.c:95: warning: 'err[1]' may be used uninitialized in this function

I'm especially concerned about "'return' with no value, in function
returning non-void".  That's a sure way to get unpredictable behavior.

Also, more testing would be great.  At least please test the
functionality you changed recently.

-- 
Regards,
Pavel Roskin

Re: Code quality

[Robert Millan]

On Wed, Apr 15, 2009 at 06:10:53PM -0400, Pavel Roskin wrote:
> Hello!
> 
> It seems to me that the code quality has decreased in the last weeks.
> In the same time, we have a growing number of compiler warnings.  It
> looks like there is a relationship between the two.
> 
> I'll appreciate if everybody who recently contributed to GRUB looks at
> the remaining warnings and fixes at least some of them.  I don't mean
> hiding the warnings, e.g. adding casts where using different types would
> be more appropriate or initializing variables gcc claims to be
> uninitialized without making sure that gcc is indeed wrong.  But if
> there is a good fix, it should be applied.

Can we start using -Werror ?  If we can't do it globally, at least for
individual modules.  This way we gradually prevent regressions in more
areas, and (hopefully) at some point get rid of them.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: Code quality

[Pavel Roskin]

On Sat, 2009-05-02 at 13:41 +0200, Robert Millan wrote:

> Can we start using -Werror ?  If we can't do it globally, at least for
> individual modules.  This way we gradually prevent regressions in more
> areas, and (hopefully) at some point get rid of them.

We should fix the warnings first.  We have some warnings where we cast a
64-bit address to a 32-bit address without doing any validation.
Turning those warnings into errors increases the risk that somebody will
add casts mindlessly instead of adding sanity checks.

Also, gcc 4.4.0 adds a bunch of warnings about aliasing.

It would be better to make the build less verbose by default to make the
warnings more visible.

-- 
Regards,
Pavel Roskin