USB: serial: io_edgeport: mark expected switch fall-throughs

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.

Quoted 6 lines · Gustavo A. R. Silva

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:

Quoted 10 lines

drivers/usb/serial/io_edgeport.c +1 -1

@@ -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) {

Quoted 1 line

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.

Sent: 28 October 2017 11:57

Quoted 6 lines

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:

drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1826

Quoted 10 lines

drivers/usb/serial/io_edgeport.c +1 -1

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

From: Bjørn Mork

Quoted 34 lines

Sent: 28 October 2017 11:57

Quoted 6 lines

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:

drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1826

Quoted 10 lines

drivers/usb/serial/io_edgeport.c +1 -1

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

Quoted 3 lines

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.

Quoted 40 lines

From: Bjørn Mork

Quoted 8 lines

Sent: 28 October 2017 11:57

Quoted 6 lines

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.

Quoted 5 lines

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:

Review static void process_rcvd_data(struct

Quoted 1 line

(quoted diff) +0 -0

@@ -1819,8 +1819,8 @@ static void process_rcvd_data(struct  

edgeport_serial *edge_serial,

Quoted 24 lines

drivers/usb/serial/io_edgeport.c static void process_rcvd_data(struct edgeport_serial *edge_serial, · line 1826

Quoted 9 lines

drivers/usb/serial/io_edgeport.c +1 -1

 					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.

Quoted 1 line

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.

This code is pretty hard to read as is and could really use some clean up...

Quoting Johan Hovold <[email protected]>:

Quoted 3 lines

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.