From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753055AbdJaFKM (ORCPT ); Tue, 31 Oct 2017 01:10:12 -0400 Received: from gateway30.websitewelcome.com ([192.185.192.34]:30971 "EHLO gateway30.websitewelcome.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751499AbdJaFKL (ORCPT ); Tue, 31 Oct 2017 01:10:11 -0400 X-Greylist: delayed 1207 seconds by postgrey-1.27 at vger.kernel.org; Tue, 31 Oct 2017 01:10:11 EDT Date: Mon, 30 Oct 2017 23:50:02 -0500 Message-ID: <20171030235002.Horde.gdw7F_ltZVrS1ZF-scG1Ps1@gator4166.hostgator.com> From: "Gustavo A. R. Silva" To: Johan Hovold Cc: David Laight , "=?utf-8?b?J0Jqw7hybg==?= Mork'" , Greg Kroah-Hartman , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs References: <20171027203906.GA7054@embeddedor.com> <87k1zf4k24.fsf@miraculix.mork.no> <063D6719AE5E284EB5DD2968C1650D6DD00A8B34@AcuExch.aculab.com> <20171030144220.GK7223@localhost> In-Reply-To: <20171030144220.GK7223@localhost> User-Agent: Horde Application Framework 5 Content-Type: text/plain; charset=utf-8; format=flowed; DelSp=Yes MIME-Version: 1.0 Content-Disposition: inline Content-Transfer-Encoding: 8bit X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - gator4166.hostgator.com X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - embeddedor.com X-BWhitelist: no X-Source-IP: 108.167.133.22 X-Source-L: Yes X-Exim-ID: 1e9OV1-0016HM-Pg X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: gator4166.hostgator.com [108.167.133.22]:30734 X-Source-Auth: garsilva@embeddedor.com X-Email-Count: 8 X-Source-Cap: Z3V6aWRpbmU7Z3V6aWRpbmU7Z2F0b3I0MTY2Lmhvc3RnYXRvci5jb20= X-Local-Domain: yes Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi David, Johan, Quoting Johan Hovold : > On Mon, Oct 30, 2017 at 11:54:33AM +0000, David Laight wrote: >> From: Bjørn Mork >> > Sent: 28 October 2017 11:57 >> > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases >> > > where we are expecting to fall through. >> > > >> > > Notice that in this particular case I replaced "...drop on through" >> > > comments with a proper "fall through" comment on its own line, which >> > > is what GCC is expecting to find. >> > >> > Sounds to me like GCC is the wrong tool for this. But I would of course >> > not mind if it was *just* the text. However, as your patch cleary >> > shows, the simplified logic leads to real problems: >> > >> > > @@ -1819,8 +1819,8 @@ static void process_rcvd_data(struct >> edgeport_serial *edge_serial, >> > > edge_serial->rxState = EXPECT_DATA; >> > > break; >> > > } >> > > - /* Else, drop through */ >> > > } >> > > + /* fall through */ >> > > case EXPECT_DATA: /* Expect data */ >> > > if (bufferLength < edge_serial->rxBytesRemaining) { >> > > rxLen = bufferLength; >> > >> > >> > The original comment clearly marked a *conditional* fall through at the >> > correct place. Your patch makes it appear as if there is an >> > unconditional fall through here. There is not. The fallthrough only >> > applies to one of a number of nested if blocks. There are no less than >> > 3 break statements in the same case block. >> > >> > Not a big deal maybe, just as the lack of any "fall through" comment >> > isn't a big deal in the first place. But this change is clearly making >> > this code harder to read, and the change is therefore harmful IMHO. >> > >> > If you can't make -Wimplicit-fallthrough work without removing such >> > precise fallthrough markings, then my proposal is to drop it and use >> > some other tool. >> >> Just remove the 'else' after the 'break' further up. > > Yeah, that might be a good way to resolve this. I was gonna suggest > adding the "fall though" comment before the case while keeping the > "Else, drop through" comment in the branch, but removing the else might > be better. > Thanks for the suggestion. > This code is pretty hard to read as is and could really use some clean > up... > I agree. I'll send a V2 of this patch and then let's see if I can help with some code refactoring. Thank you -- Gustavo A. R. Silva