From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38171) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Twyqa-0003up-60 for qemu-devel@nongnu.org; Sun, 20 Jan 2013 12:38:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TwyqY-00065h-MW for qemu-devel@nongnu.org; Sun, 20 Jan 2013 12:38:20 -0500 Message-ID: <50FC2B7A.208@suse.de> Date: Sun, 20 Jan 2013 18:38:02 +0100 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <19952e84d566d9309b15fe205db6b166f4234c33.1358697255.git.blauwirbel@gmail.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] Annotate questionable fallthroughs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: Kevin Wolf , Peter Maydell , Anthony Liguori , Markus Armbruster , Mark Langsdorf , Stefan Weil , Riku Voipio , qemu-devel , Alexander Graf , Peter Crosthwaite , Max Filippov , "qemu-ppc@nongnu.org" , Paul Brook , Paolo Bonzini , "Vassili Karpov (malc)" , Guan Xuetao , Aurelien Jarno , Richard Henderson Am 20.01.2013 18:26, schrieb Blue Swirl: > On Sun, Jan 20, 2013 at 4:56 PM, Peter Maydell wrote: >> On 20 January 2013 15:54, Blue Swirl 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 o= ffset, >>> case 0x48: /* TAR */ >>> if (s->control =3D=3D 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. >=20 > 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 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg