Re: [Qemu-devel] [PATCH] Annotate questionable fallthroughs

["Andreas Färber" <afaerber@suse.de>]

Am 20.01.2013 18:26, schrieb Blue Swirl:
> On Sun, Jan 20, 2013 at 4:56 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 20 January 2013 15:54, Blue Swirl <blauwirbel@gmail.com> wrote:
>>
>> This patch is a bit big to usefully review. A few comments on bits
>> I happened to notice:
[...]
>>> --- a/hw/stellaris.c
>>> +++ b/hw/stellaris.c
>>> @@ -182,8 +182,10 @@ static uint64_t gptm_read(void *opaque, hwaddr offset,
>>>      case 0x48: /* TAR */
>>>          if (s->control == 1)
>>>              return s->rtc;
>>> +        /* XXX: questionable fallthrough */
>>>      case 0x4c: /* TBR */
>>>          hw_error("TODO: Timer value read\n");
>>> +        /* XXX: questionable fallthrough */
>>
>> This isn't a fallthrough at all, hw_error() never returns.

Maybe hw_error() needs some annotation instead?

>> I don't think there's much point adding tons of "XXX" comments
>> when a bunch of these aren't actually wrong code. If you want to fix
>> this I think a better approach would be more focused patches aimed
>> at adding 'break;' or "/* fallthrough */" based on actual human
>> examination of the surrounding code.

+1

> The problem is that while some cases may be easy to decide, others are
> not so clear.
> 
> My initial thought about the work flow was that this patch should be
> succeeded by other patches which replace the comment with correct
> action. These could be squashed to the original patch or committed
> later. If no decision can be made for some comment, it could stay as
> XXX.

$ git grep XXX | wc --lines
75797

I don't think adding any more will help getting them addressed...

> Alternatively, I could split this patch per maintainer, architecture
> or file even. Each maintainer could tune the patches as they see fit
> and commit whatever they want later. Probably some areas would be
> never fixed.

I would suggest to split per file and to propose either action rather
than putting an XXX. I'm sure there would be static analysis volunteers
to help review, CC'ing Stefan W. and Markus. :)

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg