From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=52515 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OrurA-000056-4b for qemu-devel@nongnu.org; Sat, 04 Sep 2010 11:40:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Oruqd-0003Yy-Ri for qemu-devel@nongnu.org; Sat, 04 Sep 2010 11:40:40 -0400 Received: from mail-vw0-f45.google.com ([209.85.212.45]:42738) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Oruqd-0003Yt-MT for qemu-devel@nongnu.org; Sat, 04 Sep 2010 11:40:07 -0400 Received: by vws19 with SMTP id 19so2211387vws.4 for ; Sat, 04 Sep 2010 08:40:07 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: Date: Sat, 4 Sep 2010 17:40:06 +0200 Message-ID: Subject: Re: [Qemu-devel] [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits From: andrzej zaborowski Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: qemu-devel On 4 September 2010 16:17, Blue Swirl wrote: > Add various casts, adjust types etc. to make the warnings with > gcc flag -Wtype-limits disappear. > > Signed-off-by: Blue Swirl > --- > =C2=A0block/blkdebug.c =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A02 +- > =C2=A0hw/mips_fulong2e.c =C2=A0 =C2=A0| =C2=A0 =C2=A02 +- > =C2=A0hw/omap1.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A0= 2 +- > =C2=A0hw/ppc405_boards.c =C2=A0 =C2=A0| =C2=A0 23 +++++++++++++---------- > =C2=A0hw/ppc_newworld.c =C2=A0 =C2=A0 | =C2=A0 =C2=A03 ++- > =C2=A0hw/ppc_prep.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A03 ++- > =C2=A0hw/pxa2xx.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0 =C2=A02 +- > =C2=A0hw/sm501.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| =C2=A0 =C2=A0= 4 ++-- > =C2=A0linux-user/flatload.c | =C2=A0 =C2=A03 ++- > =C2=A0linux-user/mmap.c =C2=A0 =C2=A0 | =C2=A0 =C2=A02 +- > =C2=A0linux-user/syscall.c =C2=A0| =C2=A0 20 +++++++++++++------- > =C2=A011 files changed, 39 insertions(+), 27 deletions(-) > > diff --git a/block/blkdebug.c b/block/blkdebug.c > index 2a63df9..b5083bc 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -439,7 +439,7 @@ static void blkdebug_debug_event(BlockDriverState > *bs, BlkDebugEvent event) > =C2=A0 =C2=A0 struct BlkdebugRule *rule; > =C2=A0 =C2=A0 BlkdebugVars old_vars =3D s->vars; > > - =C2=A0 =C2=A0if (event < 0 || event >=3D BLKDBG_EVENT_MAX) { > + =C2=A0 =C2=A0if ((unsigned int)event >=3D BLKDBG_EVENT_MAX) { > =C2=A0 =C2=A0 =C2=A0 =C2=A0 return; > =C2=A0 =C2=A0 } > > diff --git a/hw/mips_fulong2e.c b/hw/mips_fulong2e.c > index cbe7156..ac82067 100644 > --- a/hw/mips_fulong2e.c > +++ b/hw/mips_fulong2e.c > @@ -258,7 +258,7 @@ static void mips_fulong2e_init(ram_addr_t > ram_size, const char *boot_device, > =C2=A0{ > =C2=A0 =C2=A0 char *filename; > =C2=A0 =C2=A0 unsigned long ram_offset, bios_offset; > - =C2=A0 =C2=A0unsigned long bios_size; > + =C2=A0 =C2=A0long bios_size; > =C2=A0 =C2=A0 int64_t kernel_entry; > =C2=A0 =C2=A0 qemu_irq *i8259; > =C2=A0 =C2=A0 qemu_irq *cpu_exit_irq; > diff --git a/hw/omap1.c b/hw/omap1.c > index 06370b6..b04aa80 100644 > --- a/hw/omap1.c > +++ b/hw/omap1.c > @@ -3675,7 +3675,7 @@ static int omap_validate_emiff_addr(struct > omap_mpu_state_s *s, > =C2=A0static int omap_validate_emifs_addr(struct omap_mpu_state_s *s, > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 target_phys_addr_= t addr) > =C2=A0{ > - =C2=A0 =C2=A0return addr >=3D OMAP_EMIFS_BASE && addr < OMAP_EMIFF_BASE= ; > + =C2=A0 =C2=A0return addr < OMAP_EMIFF_BASE; I think this only doesn't break the code because it relies on a specific value for that #define, and if the value is changed, the check has to be re-added. Same applies to other modifications in this patch. Most of the gcc warnings should not be enabled by default and I think this is one of them. They make you do some really strange things in the code just to avoid using one construct of your programming language (fully valid in the language), and in the end result in bugs of other types. It's probably ok to enable them once to see if they hint on any bugs, but if enabled by default, they'll just cause other types of bugs. There are some projects that avoid using strcat for example, because, if misused, it can cause crashes. There's even a pretty big project by Nokia that does not use malloc(), because again, "malloc can cause memory leaks". They just allocate all memory statically, and it works.. but is a pain to work with, and the program requires much more memory than needed so it doesn't run on some embedded devices. I wouldn't want to go this way. Cheers