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

From: Gustavo A. R. Silva
Date: Mon Jul 02 2018 - 09:25:55 EST


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