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

[Gustavo A. R. Silva]

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.

>>  		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.

> )
> 
>>  		case EXPECT_DATA: /* Expect data */
>>  			if (bufferLength < edge_serial->rxBytesRemaining) {
> 
> How do you compile test these these patches?
>

I already explained this above.

Thanks for your comments.
--
Gustavo