Support silencing -Wimplicit-fallthrough via comments

[carlosgalvezp]

GCC supports silencing the -Wimplicit-fallthrough warning via comments, for users that are stuck in pre-C++17. That's not the case for Clang, however. It would be good if Clang had feature parity with GCC in this regard.

Example on Compiler Explorer: https://godbolt.org/z/z94KGqaxq

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[davidbolvansky]

Hm, works in C mode.

https://godbolt.org/z/M6GzhGoaP

[carlosgalvezp]

@davidbolvansky Nope, you need to add -Wimplicit-fallthrough, it's not part of -Wall, -Wextra in Clang (it is for GCC):
https://godbolt.org/z/ddzEf36Gb

[davidbolvansky]

Ah :/ that should be fixed as well

[davidbolvansky]

https://reviews.llvm.org/rGc61401b89742f230b7e6a2cc0548fbf7442e906d

But it was reverted..

[davidbolvansky]

cc @zygoloid @AaronBallman

[AaronBallman]

https://bugs.llvm.org/show_bug.cgi?id=43465#c37 is relevant to the discussion.

[AaronBallman]

I've taken some time to review the comments in #42810 and I don't think the situation has changed all that much in the past ~2.5 years, so I think this request should be closed as a duplicate of that one.

The primary points that I think are still relevant is: there are superior ways to solve this problem in all language modes. You can use [[fallthrough]] or __attribute__((fallthrough)) in both C and C++. So the number of cases where you need to use a comment is low. Portability isn't a very strong argument as attributes are as portable as comments are. However, using comments breaks the usual expectation that code carries semantics and not comments. Further, comments are very hard to support in a way that's as consistent as attributes. The linux kernel experiment linked to in that thread demonstrates the wide variety of comments people have added to try to indicate fallthrough in real world code:

   2278 /* fall through */
    441 /* Fall through */
    253 /* fallthrough */
    164 /* FALLTHROUGH */
    116 /* fall-through */
     84 /* Fallthrough */
     33 /* FALL THROUGH */
     31 /* Fall through */				\
     27 /* FALLTHRU */
     24 /*FALLTHRU*/
     24 /* fallthru */
     19 /* fall thru */
     19 /* Fall Through */
     19 /* Fall-through */
     16 /* Else, fall through */
     15 /* fall-thru */
     13 /* Intentional fallthrough */
     13 /*FALLTHROUGH*/
     12 /* Fall thru */
     12 /* else, fall through */
     10 /*lint -fallthrough */
      9 /* Fall through. */
      9 }	/* fallthrough */
      8 /*fall through*/

That's a significant amount of variability in how people try to express intent.

(If you disagree and would like to continue the discussion, please reopen #42810 with more details so we can keep the relevant bits all in one place as best we can.)

[davidbolvansky]

So the number of cases where you need to use a comment is low. Portability isn't a very strong argument as attributes are as portable as comments are.

I would like to mention one case I reported many years ago:

westes/flex#427

But overall I agree that silencing warnings with comments is a pretty bad thing.

[AaronBallman]

So the number of cases where you need to use a comment is low. Portability isn't a very strong argument as attributes are as portable as comments are.

I would like to mention one case I reported many years ago:

westes/flex#427

Thanks! The conclusion of that thread suggests to me that flex would have an issue if we were to enable -Wimplicit-fallthrough by default. That's unfortunate given that Clang is unlikely to ever support a comment-based mechanism to silence fallthrough diagnostics.

FWIW, I'm not certain how much the logic "I am just not sure if going through all the trouble is worth it." should/will hold us back from enabling this diagnostic in -Wall or -Wextra at some point. It's not guaranteed that we'll make that change (this diagnostic does add compile time performance overhead and is somewhat of a stylistic option), but the pressure to enable it by default has been building. There are significant security issues that are caught by this diagnostic and we've been enabling security-related diagnostics lately even when they're disruptive, so I hope they're willing to reconsider their stance now that three years have passed.