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

[Johan Hovold]

On Mon, Jul 02, 2018 at 08:00:43AM -0500, Gustavo A. R. Silva wrote:
> Hi Johan,
> 
> On 07/02/2018 03:51 AM, Johan Hovold wrote:
> > On Thu, Jun 28, 2018 at 01:40:30PM -0500, Gustavo A. R. Silva wrote:
> >> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> >> where we are expecting to fall through.
> >>
> >> Signed-off-by: Gustavo A. R. Silva <gustavo@xxxxxxxxxxxxxx>
> >> ---
> >>  drivers/usb/serial/io_edgeport.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/usb/serial/io_edgeport.c b/drivers/usb/serial/io_edgeport.c
> >> index 97c69d3..441dab6 100644
> >> --- a/drivers/usb/serial/io_edgeport.c
> >> +++ b/drivers/usb/serial/io_edgeport.c
> >> @@ -1760,7 +1760,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> >>  				edge_serial->rxState = EXPECT_HDR2;
> >>  				break;
> >>  			}
> >> -			/* otherwise, drop on through */
> >> +			/* else: fall through */
> > 
> > This doesn't silence the compiler warning with gcc 7.2.0 as the "else: "
> > pattern isn't recognised.
> > 
> 
> I'm using level 2:
> 
> -Wimplicit-fallthrough=2
> 
> The thing here is that some people have pointed out that it can be misleading to
> place a plain fall-through comment after an if-else code block containing a "break".
> So, the solution above has proved to be a good one.

I don't mind the "else", but I would expect you to mention in the commit
message that you're now relying on the non-default warning level (2).

> >>  		case EXPECT_HDR2:
> >>  			edge_serial->rxHeader2 = *buffer;
> >>  			++buffer;
> >> @@ -1820,7 +1820,7 @@ static void process_rcvd_data(struct edgeport_serial *edge_serial,
> >>  					edge_serial->rxState = EXPECT_DATA;
> >>  					break;
> >>  				}
> >> -				/* Else, drop through */
> >> +				/* else: fall through */
> >>  			}
> > 
> > And this doesn't work either due to the "else: " as well as the fact
> > that the compiler expects the fallthrough comment to precede the case
> > statement directly (e.g. it would need to be moved out of the else
> > block, but that isn't necessarily desirable as we discussed last year: 
> > 
> > 	lkml.kernel.org/r/20171027203906.GA7054@xxxxxxxxxxxxxx
> > 
> 
> Yes. I'm aware of that. This certainly is still triggering a warning,
> so I just consider this
> as a temporal approach. I still need to define how are we going to
> manage cases like this.

Ok, so why did you not mention that in the commit message?

If this isn't even addressing the warning you get with the non-default
-Wimplicit-fallthrough=2, I don't see this as much of an improvement.

Might as well leave this unchanged, until all warnings in that switch
statement are addressed.

Johan