[Qemu-devel] Re: [PATCH 1/5] Suppress some gcc warnings with -Wtype-limits

[Blue Swirl]

On Sun, Sep 5, 2010 at 7:54 AM, Michael S. Tsirkin <address@hidden> wrote:
> On Sat, Sep 04, 2010 at 05:21:24PM +0000, Blue Swirl wrote:
>> In the unsigned number space, the checks can be merged into one,
>> assuming that BLKDBG_EVEN_MAX is less than INT_MAX. Alternatively we
>> could have:
>>  -    if (event < 0 || event >= BLKDBG_EVENT_MAX) {
>>  +    if ((int)event < 0 || event >= BLKDBG_EVENT_MAX) {
>>
>> This would also implement the check that the writer of this code was
>> trying to make.
>> The important thing to note is however is that the check as it is now
>> is not correct.
>
> I agree. But it seems to indicate a bigger problem.
>
> If we are trying to pass in a negative value, which is not one
> of enum values, using BlkDebugEvent as type is just confusing,
> we should just pass int instead.

AFAICT it's only possible to use the values listed in event_names in
blkdebug.c, other values are rejected. So the check should actually be
an assert() or it could even be removed.

>> >> How about adding assert(OMAP_EMIFS_BASE == 0) and commenting out the
>> >> check? Then if the value changes, the need to add the comparison back
>> >> will be obvious.
>> >
>> > This would work but it's weird.  The thing is it's currently a correct
>> > code and the check may be useless but it's the optimiser's task to
>> > remove it, not ours.  The compiler is not able to tell whether the
>> > check makes sense or nott, because the compiler only has access to
>> > preprocessed code.  So why should you let the compiler have anything
>> > to say on it.
>>
>> Good point. I'll try to invent something better.
>
> Use #pragma to supress the warning? Maybe we could wrap this in a macro ..

Those lines may also desynch silently with changes to OMAP_EMIFS_BASE.

I think the assertion is still the best way, it ensures that something
will happen if OMAP_EMIFS_BASE changes. We could for example remove
OMAP_EMIFS_BASE entirely (it's only used for the check), but someone
adding a new define could still forget to adjust the check anyway.