Message ID | 1284127451-22092-2-git-send-email-Jes.Sorensen@redhat.com |
---|---|
State | New |
Headers | show |
On Fri, Sep 10, 2010 at 2:04 PM, <Jes.Sorensen@redhat.com> wrote: > From: Jes Sorensen <Jes.Sorensen@redhat.com> > > Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com> > --- > block/blkdebug.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/block/blkdebug.c b/block/blkdebug.c > index 2a63df9..17d796d 100644 > --- a/block/blkdebug.c > +++ b/block/blkdebug.c > @@ -439,7 +439,12 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkDebugEvent event) > struct BlkdebugRule *rule; > BlkdebugVars old_vars = s->vars; > > - if (event < 0 || event >= BLKDBG_EVENT_MAX) { > + /* > + * enum is not guaranteed to be signed on all archs, so cast to > + * int before the comparison against zero to avoid compiler > + * warning when building with -Wtype-limits > + */ > + if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) { I changed 'if' to 'assert' in my version because the check could only fail due to an internal error: http://lists.nongnu.org/archive/html/qemu-devel/2010-09/msg00239.html There's also Michael's version with a cast to unsigned int. It's a bit simpler, the generated code is the same: $ cat check.c int f(int x) { if (x < 0 || x > 1000) return 1; return 0; } int g(int x) { if ((unsigned)x > 1000) return 1; return 0; } $ gcc -O -S check.c $ head -20 check.s .file "check.c" .text .globl f .type f, @function f: .LFB2: cmpl $1000, %edi seta %al movzbl %al, %eax ret .LFE2: .size f, .-f .globl g .type g, @function g: .LFB3: cmpl $1000, %edi seta %al movzbl %al, %eax ret
On 09/10/10 19:05, Blue Swirl wrote: > On Fri, Sep 10, 2010 at 2:04 PM, <Jes.Sorensen@redhat.com> wrote: >> From: Jes Sorensen <Jes.Sorensen@redhat.com> >> >> Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com> >> --- >> block/blkdebug.c | 7 ++++++- >> 1 files changed, 6 insertions(+), 1 deletions(-) >> >> diff --git a/block/blkdebug.c b/block/blkdebug.c >> index 2a63df9..17d796d 100644 >> --- a/block/blkdebug.c >> +++ b/block/blkdebug.c >> @@ -439,7 +439,12 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkDebugEvent event) >> struct BlkdebugRule *rule; >> BlkdebugVars old_vars = s->vars; >> >> - if (event < 0 || event >= BLKDBG_EVENT_MAX) { >> + /* >> + * enum is not guaranteed to be signed on all archs, so cast to >> + * int before the comparison against zero to avoid compiler >> + * warning when building with -Wtype-limits >> + */ >> + if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) { > > I changed 'if' to 'assert' in my version because the check could only > fail due to an internal error: > http://lists.nongnu.org/archive/html/qemu-devel/2010-09/msg00239.html Sorry I missed your posting. I am happy with your version too, ACK from me. I did a pull before doing this patch and didn't see it, which is why I posted my version, but your patch does the trick nicely. Cheers, Jes
diff --git a/block/blkdebug.c b/block/blkdebug.c index 2a63df9..17d796d 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -439,7 +439,12 @@ static void blkdebug_debug_event(BlockDriverState *bs, BlkDebugEvent event) struct BlkdebugRule *rule; BlkdebugVars old_vars = s->vars; - if (event < 0 || event >= BLKDBG_EVENT_MAX) { + /* + * enum is not guaranteed to be signed on all archs, so cast to + * int before the comparison against zero to avoid compiler + * warning when building with -Wtype-limits + */ + if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) { return; }