Messages in this thread | | | Date | Sat, 28 Oct 2017 18:47:39 -0500 | From | "Gustavo A. R. Silva" <> | Subject | Re: [PATCH] USB: serial: io_edgeport: mark expected switch fall-throughs |
| |
Quoting Bjørn Mork <bjorn@mork.no>:
> "Gustavo A. R. Silva" <garsilva@embeddedor.com> writes: > >> 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. >
I see. You are right.
> 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. >
I will talk with the hardening guys to see what we can do about this.
I appreciate for your comments. Thanks -- Gustavo A. R. Silva
| |