Re: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs

[Gustavo A. R. Silva]

Hi David, Johan,

Quoting Johan Hovold <johan@xxxxxxxxxx>:

> 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