Re: [Qemu-devel] Re: [PATCH] tcg: Fix compiler error (comparison of unsigned expression)

[Stefan Weil]

Am 08.10.2010 23:43, schrieb Stefan Weil:
> Am 08.10.2010 18:57, schrieb Hollis Blanchard:
> > On Fri, Oct 8, 2010 at 1:32 AM, Stefan Weil <address@hidden> 
> > wrote:
> > > When qemu is configured with --enable-debug-tcg,
> > > gcc throws this warning (or error with -Werror):
> > >
> > > tcg/tcg.c:1030: error: comparison of unsigned expression >= 0 is 
> > > always true
> > >
> > > Fix it by removing the >= 0 part.
> > > The type cast to 'unsigned' catches negative values of op
> > > (which should never happen).
> > >
> > > This is a modification of Hollis Blanchard's patch.
> > >
> > > Cc: Hollis Blanchard <address@hidden>
> > > Cc: Blue Swirl <address@hidden>
> > > Signed-off-by: Stefan Weil <address@hidden>
> > > ---
> > >  tcg/tcg.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/tcg/tcg.c b/tcg/tcg.c
> > > index e0a9030..0cdef0d 100644
> > > --- a/tcg/tcg.c
> > > +++ b/tcg/tcg.c
> > > @@ -1027,7 +1027,7 @@ void tcg_add_target_add_op_defs(const 
> > > TCGTargetOpDef *tdefs)
> > >         if (tdefs->op == (TCGOpcode)-1)
> > >             break;
> > >         op = tdefs->op;
> > > -        assert(op >= 0 && op < NB_OPS);
> > > +        assert((unsigned)op < NB_OPS);
> > >         def = &tcg_op_defs[op];
> > >  #if defined(CONFIG_DEBUG_TCG)
> > >         /* Duplicate entry in op definitions? */
> >
> > According to the warning, op is already unsigned, so this simply
> > removes the >=0 test, which was my original patch.
> >
> > In contrast, Blue wanted a cast to int, as seen in
> > 95ee3914bfd551aeec49932a400530141865acad.
> >
> > -Hollis
>
> I read the original thread. Michael already proposed the
> unsigned cast in that thread.
>
> Your patch is correct as long as we use gcc and as long
> as gcc uses unsigned enums when possible (it is possible here).
>
> Other compilers or new versions of gcc might use signed enums.
> For those (and also for current gcc versions), my patch works.
> Blue's solution also works but is a bit more code without
> having advantages.
>
> There are even more solutions which are accepted by the
> compiler: ((op > first && op <= last) || (op == first))
> with first, last being the first or last enum member.
>
> - Stefan

Hollis, do you still see problems with my patch?
Or can it be committed?

Regards,
Stefan