Consider this C code: ------------------- #include <stdio.h> #include <stdlib.h> int main (void) { int x = random(); switch (x) { case 1: printf("case1"); /*FALLTHROUGH*/ default: printf("default"); } return 0; } ------------------- clang trunk (but not the current release, 9.0) with -Wimplicit-fallthrough warns: ------------------- <source>:12:5: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough] default: ^ <source>:12:5: note: insert '__attribute__((fallthrough));' to silence this warning default: ^ __attribute__((fallthrough)); <source>:12:5: note: insert 'break;' to avoid fall-through default: ^ break; ------------------- gcc 7.x and later do *not* warn, because they detect the comment "/*FALLTHROUGH*/" and take it as a cue that it's deliberately. This is documented here: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wimplicit-fallthrough Compare on godbolt: https://godbolt.org/z/UFpfNm It would be nice if clang did the same. Especially since the venerable flex <https://github.com/westes/flex> generates code that uses exactly that comment. Without this, clang will start giving false positive warnings (that gcc does not) on lots of flex-generated code out there.
IMHO, flex should use __attribute__((fallthrough)) rather than weird /*FALLTHROUGH*/ comments..
Thanks for your reply. Changing flex is perhaps a possibility, but of course not mutually exclusive to changing clang. Could you suggest exactly how to suppress it in a portable manner? This: https://clang.llvm.org/docs/LanguageExtensions.html#has-c-attribute suggests guarding with: #if __has_c_attribute(fallthrough) but it doesn't work: https://godbolt.org/z/sTcybc
hmm, it works in C++ mode, not in C mode.. If you add -fdouble-square-bracket-attributes in C mode, it works. Yeah, we have a bug here..
-std=c2x fixes it too.
Probably needs to be fixed for 9.0.1
(In reply to David Bolvansky from comment #3) > hmm, it works in C++ mode, not in C mode.. > > If you add -fdouble-square-bracket-attributes in C mode, it works. > > Yeah, we have a bug here.. I'm not seeing the bug. __has_c_attribute tests whether we support [[]] attributes. Given the command line arguments in the godbolt link, we do not support the attribute with that spelling. If you enable double square brackets, then it does support the attribute. __has_attribute tests whether we support __attribute__(()) attributes. The test should be something more like: #if defined(__has_attribute) && __has_attribute(fallthrough) __attribute__((fallthrough)) #elif defined(__has_c_attribute) && __has_c_attribute(fallthrough) [[fallthrough]] #endif (probably hidden behind a few layers of macros).
Ah, right. Totally forgot about __has_attribute.
Aaron, so the docs here are wrong then? https://clang.llvm.org/docs/LanguageExtensions.html#has-c-attribute I don't get in the branch at all: https://godbolt.org/z/suqqGw
FWIW, I think this is a regression in behavior for the clang implicit fallthrough check. Clang 9 does not diagnose this code, Clang trunk does diagnose it, and the user has made their intent clear using a comment that's been checked by tools for decades for exactly this purpose. I think the bug is that we disabled support for comment-based fallthrough diagnostic suppression. I think we should bring that functionality back.
(In reply to Sean McBride from comment #8) > Aaron, so the docs here are wrong then? > https://clang.llvm.org/docs/LanguageExtensions.html#has-c-attribute I don't see what's wrong with them. "This function-like macro takes a single argument that is the name of an attribute exposed with the double square-bracket syntax in C mode." > I don't get in the branch at all: > https://godbolt.org/z/suqqGw The compiler options provided don't have double square-bracket syntax supported. Check out the command line flags here, which show the differences: https://godbolt.org/z/XQtSc6
Aaron, are you sure clang ever diagnosed fallthrough *in C* before trunk? Playing with godbolt, I can't get any fallthrough warnings in any version before trunk. And I looked, but did not find, any clang docs that say detection of magic comments exists or ever existed. As for those docs, ok, I think I get it: I want __has_attribute. The __has_c_attribute's example of fallthrough was blinding me. :) I tried many compilers on godbolt, and indeed this seems to work everywhere: #if defined(__has_attribute) #if __has_attribute(fallthrough) __attribute__((fallthrough)); #endif #endif
(In reply to Sean McBride from comment #11) > Aaron, are you sure clang ever diagnosed fallthrough *in C* before trunk? > Playing with godbolt, I can't get any fallthrough warnings in any version > before trunk. Ahh, shoot, you're right, I missed that we had this disabled in C previously, not that we used to support this suppression previously. > And I looked, but did not find, any clang docs that say detection of magic > comments exists or ever existed. I agree. I've updated the fields for the bug accordingly, thank you for pointing that out. > As for those docs, ok, I think I get it: I want __has_attribute. The > __has_c_attribute's example of fallthrough was blinding me. :) > > > I tried many compilers on godbolt, and indeed this seems to work everywhere: > > #if defined(__has_attribute) > #if __has_attribute(fallthrough) > __attribute__((fallthrough)); > #endif > #endif Yes, that should work on many different compilers (though not all, like MSVC).
So, to recap: - gcc 7.1 (May 2017) and later can warn on implicit fallthrough *in C*. - no public release of clang warns on implicit fallthrough *in C*, but such a warning is now in trunk. - gcc allows suppressing false positives with an attribute or magic comment. - clang trunk allows suppressing with the same attribute, but not magic comments. In the ~2.5 years since gcc 7.1, some C codebases have been using magic comments to suppress false positives. Notably all code generated by the *current* version of flex uses the magic comment "/*FALLTHROUGH*/". In general, clang strives to minimize false positives. Since no clang has shipped with this warning yet, supporting the same magic comments would spare people from re-supressing these warnings all over again with a different technique.
(In reply to Sean McBride from comment #13) > So, to recap: > > - gcc 7.1 (May 2017) and later can warn on implicit fallthrough *in C*. > - no public release of clang warns on implicit fallthrough *in C*, but such > a warning is now in trunk. > > - gcc allows suppressing false positives with an attribute or magic comment. > - clang trunk allows suppressing with the same attribute, but not magic > comments. > > In the ~2.5 years since gcc 7.1, some C codebases have been using magic > comments to suppress false positives. Notably all code generated by the > *current* version of flex uses the magic comment "/*FALLTHROUGH*/". > > In general, clang strives to minimize false positives. > > Since no clang has shipped with this warning yet, supporting the same magic > comments would spare people from re-supressing these warnings all over again > with a different technique. +1
Just a little ping... Sure would be nice to have this fixed before clang 10 ships and introduces this false positive.
(In reply to Sean McBride from comment #15) > Just a little ping... > > Sure would be nice to have this fixed before clang 10 ships and introduces > this false positive. It would, but it requires making nontrivial changes to the analysis that I don't have time to work on currently. The analysis uses attributed statements to map which fallthroughs have been handled in the CFG and we need to add support for reaching back into the source to find the original text to see if there's a comment nearby (which is not an attributed statement). If you wanted to take a stab at fixing it, I would be happy to review a patch. The code is mostly in FallthroughMapper::checkFallThroughInfoBlock() in AnalysisBasedWarnings.cpp.
https://reviews.llvm.org/D73852
Nice work!
Patch pushed (398b4ed87d488b42032c8d0304324dce76ba9b66). Since this is set to block 10.0 release, I'll leave this open for whoever knows what to do with that.
I've commented on the patch. I'm not sure this is a good idea, so holding off merging it to 10.x for now.
https://reviews.llvm.org/D64838 line 1242 on the lhs is what enabled the warning in C. From the commit description, it sounds like this wasn't intentional. So I think for now we should go back to the world where the warning is off by default in C.
(In reply to Nico Weber from comment #21) > https://reviews.llvm.org/D64838 line 1242 on the lhs is what enabled the > warning in C. > > From the commit description, it sounds like this wasn't intentional. > > So I think for now we should go back to the world where the warning is off > by default in C. Good news: we never left that world. The warning is off by default in both C and C++ already.
> Good news: we never left that world. The warning is off by default in both C > and C++ already. So what's the problem then? :-) flex generated code probably doesn't want to compile with -Wimplicit-fallthrough since it relies on exactly that..
(In reply to Hans Wennborg from comment #23) > > Good news: we never left that world. The warning is off by default in both C > > and C++ already. > > So what's the problem then? :-) > > flex generated code probably doesn't want to compile with > -Wimplicit-fallthrough since it relies on exactly that.. That's an excellent point, but the most compatible thing Flex can generate are comments. __attribute__((fallthrough)) does not work until recent versions of Clang. [[fallthrough]] is C2x. /* FALLTHROUGH */ comments will cause no bad behavior for anyone. Forcing everyone to identify their flex output to disable a compiler warning that's on everywhere else in the project is a bit obnoxious, which is the problem this patch solves.
>flex generated code probably doesn't want to compile with >-Wimplicit-fallthrough since it relies on exactly that.. Flex generated code does not rely on exactly that. It uses one of gcc's longstanding & documented means of indicating deliberate fall through, namely the magic comment. For 2.5 years gcc has had -Wimplicit-fallthrough in C, clang 10 will be the first to support it in C. Basically, it would be nice for clang to match gcc wrt this warning, since it's the one catching up. There's possibly a lot of C codebases out there that chose to annotate with a magic comment. Flex is a major example. For such codebases, clang 10 would add a bunch of false positive warnings without this fixed. See also comment #13 https://bugs.llvm.org/show_bug.cgi?id=43465#c13
Note that another slight GCC incompatibility is that -Wimplicit-fallthrough is actually part of -Wextra with GCC : https://godbolt.org/z/qEUwF-
Richard, do you have any opinions on this?
I think we should remove -Wimplicit-fallthrough from -Wextra in language modes that don't have `[[fallthrough]]` as a standard feature. We shouldn't have warnings in -Wextra that can only be suppressed by using a vendor extension.
(In reply to Richard Smith from comment #28) > I think we should remove -Wimplicit-fallthrough from -Wextra in language > modes that don't have `[[fallthrough]]` as a standard feature. We shouldn't > have warnings in -Wextra that can only be suppressed by using a vendor > extension. -Wimplicit-fallthrough isn't enabled in -Wextra, so I guess that's more good news. :-) https://godbolt.org/z/knXaER
It looks like we're not doing anything more for 10.x here, so I'll unblock the tracking bug. https://reviews.llvm.org/D73852 is still in master though, so unless someone decides otherwise, clang 11 is shipping with support for this warning-suppressing comment regex.
1) The only way to enable the implicit fallthrough check currently is by manually specifying -Wimplicit-fallthrough on the command line. It is not enabled by -Wall or -Wextra. https://godbolt.org/z/2tnnRZ 2) Clang 9.0 and earlier disabled -Wimplicit-fallthrough support for C code because there was no way to suppress the diagnostics. (We've since added support for __attribute__((fallthrough)), however.) https://godbolt.org/z/okVt5q 3) GCC supports suppressing fallthrough via comments and this mechanism is used by some libraries that generate code, such as flex. Clang 10.0 currently does not support this, but Clang trunk does. https://godbolt.org/z/E2nf9U The questions, to me, are: * Do we want to support using a comment to suppress the fallthrough diagnostic? * Do we want to support that in Clang 10.0? My answers are "yes" to both questions. We want to support the feature because GCC supports it similarly and it's functionality is assumed by at least one common third-party library. In Clang 9.0, we did not warn on this generated code because we disabled the diagnostic entirely. In Clang 10.0, we will start to warn on this code. Users can use __attribute__((fallthrough)) in Clang 10.0, so they will have some way to silence the diagnostic, but it won't help for the third-party generated code case.
Hans, you mentioned weeks ago "I'm not sure this is a good idea, so holding off..." but what is the actual argument against the patch? I've reread the comments here but I don't see one. How about just removing -Wimplicit-fallthrough support (in C) in Clang 10 then? It can be restored in Clang 11 when the suppression works. If this ships in Clang 10, codebases will need a preprocessor soup to suppress false positives that they already suppressed years ago. Clang is copying a gcc warning, but not copying how it's suppressed.
(In reply to Sean McBride from comment #32) > Hans, you mentioned weeks ago "I'm not sure this is a good idea, so holding > off..." but what is the actual argument against the patch? I've reread the > comments here but I don't see one. My understanding of the arguments are: * Using comments to suppress diagnostics is novel within the compiler, though it is supported elsewhere (like NOLINT comments in clang-tidy) * We don't 100% match the GCC behavior for what kind of comments will suppress the diagnostic. I think those concerns were worth thinking about more closely, so I support Hans wanting to wait before changing the release branch. However, I think we've had sufficient time to consider it and need to make a decision before we release 10.0. > How about just removing -Wimplicit-fallthrough support (in C) in Clang 10 > then? It can be restored in Clang 11 when the suppression works. That is another option, but given that we also added support for __attribute__((fallthrough)) to Clang 10 specifically so that we could enable this diagnostic in C, that seems a bit weird -- we give people a way to suppress a diagnostic they can't enable. However, I suppose you could still use __attribute__((fallthrough)) in C++98 mode if you really didn't want to use the [[fallthrough]] as an extension for some reason. > If this ships in Clang 10, codebases will need a preprocessor soup to > suppress false positives that they already suppressed years ago. Clang is > copying a gcc warning, but not copying how it's suppressed. This is what I want to avoid.
* Do we want to support using a comment to suppress the fallthrough diagnostic? * Do we want to support that in Clang 10.0? 2 x Yes. We should cherry-pick the patch which allows us to supress warning using a comment.
I want to tag 10.0.0-rc3 today, with the intention that it will be the last release candidate. It seems there hasn't been any progress in the discussion around this warning. I double checked that it doesn't fire under -Wall or -Wextra on the 10.x branch. I think that's good enough that we don't have to rush any decisions for the branch.
(In reply to Hans Wennborg from comment #35) > I want to tag 10.0.0-rc3 today, with the intention that it will be the last > release candidate. > > It seems there hasn't been any progress in the discussion around this > warning. I double checked that it doesn't fire under -Wall or -Wextra on the > 10.x branch. I think that's good enough that we don't have to rush any > decisions for the branch. There were several requests to add this patch in to 10.0 because of a behavioral regression with a common third-party library when -Wimplicit-fallthrough is enabled in C. I've not seen any evidence that we should not add this patch to 10.0 aside from a mild fear that someone doesn't like using comments to disable diagnostics in precisely the same way as GCC (though we do cover the third-party library case). I this this patch should be brought in to 10.0 as requested, but if Richard feels differently, I'll live with his decision.
Facts: ====== Clang warns under -Wimplicit-fallthrough on implicit fallthrough that is not annotated with [[fallthrough]] or __builtin_fallthrough(). When enabled, this leads to warnings, such as in code generated by flex. The flex maintainers seem reluctant to add such an annotation, despite the attribute form being standard in C++17 and C20 (https://github.com/westes/flex/issues/427). (Perhaps they would consider adding a flag to optionally use the attribute, but it’s not their problem -- people enabling -Wimplicit-fallthrough are applying a constraint to their code that it was never meant to satisfy.) -Wimplicit-fallthrough is not controlled by any other warning flag in Clang. Anyone seeing these warnings explicitly opted into them. In GCC, it’s controlled by -Wextra, and (perhaps because of that) GCC suppresses the warning if it sees a ‘fallthrough’ comment (which Flex generates, and is a widely-used convention in codebases that can’t rely on [[fallthrough]];). Between Clang 9 and Clang 10, we started respecting -Wimplicit-fallthrough even in C (because we implement [[fallthrough]]; in C now). That results in an apparent regression for code that enables -Wimplicit-fallthrough in C (perhaps because they also build with GCC and it does something useful there). The history is not particularly relevant, but contrary to some claims, -Wimplicit-fallthrough is not a GCC extension that Clang copied and modified. It’s a Clang extension (added 2012-05) that GCC copied and modified (in 2016-09). Due to a technical process failure, the patch implementing the comment recognition (https://reviews.llvm.org/D73852) did not go to cfe-commits for review, and the submitted patch didn’t go to cfe-commits either. We need to decide what to do in Clang trunk, and what to do in Clang 10. For Clang 10: ============= I think the motivation to treat this as a release blocker is weak at best. People who get GCC’s fallthrough warning via -Wextra do not get Clang’s warning, so there’s no problem for them. People who explicitly ask for it via -Wimplicit-fallthrough can turn that warning off for code they don’t control, as they should in general expect to do for *any* warning that enforces a particular coding style that the generated code does not follow. We do not have a properly-reviewed patch to merge, the patch we do have is too complex to merge at this very late stage, and (due to the limited audience on the review) I don’t think we have sufficient consensus that we want to go in that direction. My code owner position is: merging this patch to Clang 10 is not acceptable. Re-disabling -Wimplicit-fallthrough in C for Clang 10 is acceptable (but I personally consider it undesirable, as it would lose us functionality we intended to add in Clang 10 and doesn’t pave a path for solving this in Clang 11). For Clang trunk: ================ First off: the patch didn’t receive sufficient review. Please revert it for now. Then we need to have a design discussion about what we actually want going forward. In favor of recognizing comments: --------------------------------- * It solves an immediate problem for people who can’t or won’t change their code to add an annotation, and can’t or won’t change their warning flags to remove -Wimplicit-fallthrough. Against recognizing comments: ----------------------------- * The historical agreement between the compiler and the programmer regarding comments is simple: comments don’t affect the interpretation of the code. This breaks that agreement. That means that (for example) tools can’t automatically strip comments any more (simple tools that determine whether a recompile is necessary by stripping comments and comparing are broken by this). * If we ever want to transition C and C++ to a place where -Wimplicit-fallthrough is an error by default (ideally an error required by the relevant standards), we need to move away from using comments to indicate fallthrough. - The intent of Clang’s -Wimplicit-fallthrough was always that people using this warning would transition from comment-style fallthrough annotations to the attribute, and the warning would help them do that. Recognizing comments breaks an originally-intended use of the warning flag. * Comments are not checkable in the same way that attributes and other annotations are. Clang will diagnose a fallthrough attribute where fallthrough cannot happen but will not diagnose a similar comment. Regarding the current patch: ---------------------------- * The patch we do have employs a novel and surprising approach (applying a regex to a line of source text), and as a result does not correctly detect comments, matches inside string literals, can match on the wrong side of an if-else or other control flow, and so on. We largely get away with these bugs because it only looks at a single non-empty source line; however, that too leads to false negatives (for example, it allows f1 but not the more common f2 here: https://godbolt.org/z/Y2vQ2H). My opinion: ----------- (Personal opinion, not code owner position.) The option that I find most preferable is the current state of Clang 10. -Wimplicit-fallthrough enforces a particular code style, and that code style uses attributes (rather than comments) to indicate fallthrough. It’s unfortunate that GCC implemented something else under the same flag name, but we have little control over that. As with any style warning, it should not be turned on in code that’s not intended to follow that code style. But the warning flag is already used on several large C++ codebases that do not want comments to suppress the warning, and we would be doing a disservice to those users if we change the meaning now -- they would lose any way to enforce their intended style.
(In reply to Richard Smith from comment #37) > Facts: > ====== > > Clang warns under -Wimplicit-fallthrough on implicit fallthrough that is not > annotated with [[fallthrough]] or __builtin_fallthrough(). When enabled, > this leads to warnings, such as in code generated by flex. The flex > maintainers seem reluctant to add such an annotation, despite the attribute > form being standard in C++17 and C20 > (https://github.com/westes/flex/issues/427). (Perhaps they would consider > adding a flag to optionally use the attribute, but it’s not their problem -- > people enabling -Wimplicit-fallthrough are applying a constraint to their > code that it was never meant to satisfy.) > > -Wimplicit-fallthrough is not controlled by any other warning flag in Clang. > Anyone seeing these warnings explicitly opted into them. In GCC, it’s > controlled by -Wextra, and (perhaps because of that) GCC suppresses the > warning if it sees a ‘fallthrough’ comment (which Flex generates, and is a > widely-used convention in codebases that can’t rely on [[fallthrough]];). > > Between Clang 9 and Clang 10, we started respecting -Wimplicit-fallthrough > even in C (because we implement [[fallthrough]]; in C now). That results in > an apparent regression for code that enables -Wimplicit-fallthrough in C > (perhaps because they also build with GCC and it does something useful > there). > > The history is not particularly relevant, but contrary to some claims, > -Wimplicit-fallthrough is not a GCC extension that Clang copied and > modified. It’s a Clang extension (added 2012-05) that GCC copied and > modified (in 2016-09). > > Due to a technical process failure, the patch implementing the comment > recognition (https://reviews.llvm.org/D73852) did not go to cfe-commits for > review, and the submitted patch didn’t go to cfe-commits either. I'm sorry for my part in that technical process failure. :-( I agree with all of this, with one additional fact: we also added support for __attribute__((fallthrough)) in C mode in Clang 10.0, so there are two ways to disable this with attributes in C in Clang 10.0 even without comment support. > We need to decide what to do in Clang trunk, and what to do in Clang 10. > > For Clang 10: > ============= > > I think the motivation to treat this as a release blocker is weak at best. > People who get GCC’s fallthrough warning via -Wextra do not get Clang’s > warning, so there’s no problem for them. People who explicitly ask for it > via -Wimplicit-fallthrough can turn that warning off for code they don’t > control, as they should in general expect to do for *any* warning that > enforces a particular coding style that the generated code does not follow. > > We do not have a properly-reviewed patch to merge, the patch we do have is > too complex to merge at this very late stage, and (due to the limited > audience on the review) I don’t think we have sufficient consensus that we > want to go in that direction. > > My code owner position is: merging this patch to Clang 10 is not acceptable. > Re-disabling -Wimplicit-fallthrough in C for Clang 10 is acceptable (but I > personally consider it undesirable, as it would lose us functionality we > intended to add in Clang 10 and doesn’t pave a path for solving this in > Clang 11). I would also consider it undesirable to disable -Wimplicit-fallthrough in C given that we've put in efforts to support it explicitly and those efforts work regardless of using comments to suppress. I think this means that Clang 10.0 needs no further effort in this area and Hans can clear this bug as a blocker for the release. > For Clang trunk: > ================ > > First off: the patch didn’t receive sufficient review. Please revert it for > now. Then we need to have a design discussion about what we actually want > going forward. SGTM -- Luboš, will you take care of reverting 398b4ed87d48 from master? > In favor of recognizing comments: > --------------------------------- > > * It solves an immediate problem for people who can’t or won’t change their > code to add an annotation, and can’t or won’t change their warning flags to > remove -Wimplicit-fallthrough. > > Against recognizing comments: > ----------------------------- > > * The historical agreement between the compiler and the programmer regarding > comments is simple: comments don’t affect the interpretation of the code. > This breaks that agreement. That means that (for example) tools can’t > automatically strip comments any more (simple tools that determine whether a > recompile is necessary by stripping comments and comparing are broken by > this). > > * If we ever want to transition C and C++ to a place where > -Wimplicit-fallthrough is an error by default (ideally an error required by > the relevant standards), we need to move away from using comments to > indicate fallthrough. > > - The intent of Clang’s -Wimplicit-fallthrough was always that people > using this warning would transition from comment-style fallthrough > annotations to the attribute, and the warning would help them do that. > Recognizing comments breaks an originally-intended use of the warning flag. > > * Comments are not checkable in the same way that attributes and other > annotations are. Clang will diagnose a fallthrough attribute where > fallthrough cannot happen but will not diagnose a similar comment. > > Regarding the current patch: > ---------------------------- > > * The patch we do have employs a novel and surprising approach (applying a > regex to a line of source text), and as a result does not correctly detect > comments, matches inside string literals, can match on the wrong side of an > if-else or other control flow, and so on. We largely get away with these > bugs because it only looks at a single non-empty source line; however, that > too leads to false negatives (for example, it allows f1 but not the more > common f2 here: https://godbolt.org/z/Y2vQ2H). This is a strong technical argument against the comment patch on trunk, thank you for raising it. > My opinion: > ----------- > > (Personal opinion, not code owner position.) The option that I find most > preferable is the current state of Clang 10. -Wimplicit-fallthrough enforces > a particular code style, and that code style uses attributes (rather than > comments) to indicate fallthrough. It’s unfortunate that GCC implemented > something else under the same flag name, but we have little control over > that. As with any style warning, it should not be turned on in code that’s > not intended to follow that code style. But the warning flag is already used > on several large C++ codebases that do not want comments to suppress the > warning, and we would be doing a disservice to those users if we change the > meaning now -- they would lose any way to enforce their intended style. I'm not certain I agree with all of the points you raise in the pros/cons, but I also don't feel strongly enough in favor of using comments to suppress diagnostics in the compiler to argue further for it in light of the technical points you raised. I would be perfectly content with the state of Clang 10.0 in its current state.
Thanks everyone, I will not make any changes regarding this for the 10.x branch.
(In reply to Richard Smith from comment #37) > I think the motivation to treat this as a release blocker is weak at best. > People who get GCC’s fallthrough warning via -Wextra do not get Clang’s > warning, so there’s no problem for them. People who explicitly ask for it > via -Wimplicit-fallthrough can turn that warning off for code they don’t > control, as they should in general expect to do for *any* warning that > enforces a particular coding style that the generated code does not follow. - The code does follow the coding style as expected by GCC, so this is GCC incompatibility. - Sources generated from flex are usually a mix of flex code and handwritten code, so people do control at least part of it, and possibly they would like to get warnings there too. > First off: the patch didn’t receive sufficient review. Please revert it for > now. Done. > In favor of recognizing comments: > --------------------------------- > > * It solves an immediate problem for people who can’t or won’t change their > code to add an annotation, and can’t or won’t change their warning flags to > remove -Wimplicit-fallthrough. * It's a special case for the the legacy cases listed above, and it won't hurt any code that will not use the comment. > Against recognizing comments: > ----------------------------- > > * The historical agreement between the compiler and the programmer regarding > comments is simple: comments don’t affect the interpretation of the code. > This breaks that agreement. That means that (for example) tools can’t > automatically strip comments any more (simple tools that determine whether a > recompile is necessary by stripping comments and comparing are broken by > this). Then that agreement has already been broken, e.g. by the do-comments-nest-or-not case (https://stackoverflow.com/questions/6698039/nested-comments-in-c-c#6698115). And if by those tools you mean ccache then its lets-be-smart-about-removing-comments feature was dropped in its latest version because of being unreliable (e.g. raw strings make comment dropping apparently rather difficult). > * If we ever want to transition C and C++ to a place where > -Wimplicit-fallthrough is an error by default (ideally an error required by > the relevant standards), we need to move away from using comments to > indicate fallthrough. Irrelevant for a legacy support feature. > Regarding the current patch: > ---------------------------- > > * The patch we do have employs a novel and surprising approach (applying a > regex to a line of source text), and as a result does not correctly detect > comments, matches inside string literals, can match on the wrong side of an > if-else or other control flow, and so on. We largely get away with these > bugs because it only looks at a single non-empty source line; however, that > too leads to false negatives (for example, it allows f1 but not the more > common f2 here: https://godbolt.org/z/Y2vQ2H). The code cannot match inside string literals, because it checks after the last statement before next case statement. The control flow bug is an interesting one, with 'LastStmt' not exactly being the last one, but that presumably can be fixed. Also, the patch does not need to be perfect, and was never meant to be. It should be good enough to reasonably cover existing legacy code.
Well, now that you've decided to ship this in Clang 10, I personally no longer care to have it ever fixed, since my codebases will need to support Clang 10 for years to come, I'll need the compiler-specific suppressions anyway, so having it fixed in Clang 11 is pointless. So, as far as I'm concerned, you can just close this ticket. Thanks for your consideration and work on this all the same, especially Aaron for your patch!
Closing as WONTFIX. (In reply to Sean McBride from comment #41) > Well, now that you've decided to ship this in Clang 10, I personally no > longer care to have it ever fixed, since my codebases will need to support > Clang 10 for years to come, I'll need the compiler-specific suppressions > anyway, so having it fixed in Clang 11 is pointless. So, as far as I'm > concerned, you can just close this ticket. > > Thanks for your consideration and work on this all the same, especially > Aaron for your patch! Happy to help, but Luboš was the patch author (https://bugs.llvm.org/show_bug.cgi?id=43465#c17), and I agree: thank you for the patch!
Whoops. Thanks Luboš!
See also https://lore.kernel.org/lkml/a588204afbfe4c8dd56d0cb00d8e6e14dc561a62.camel@perches.com/ where Joe Perches who maintains the Linux kernel's patch validator (scripts/checkpatch.pl) wrote a tool to convert all of the Linux kernel's comments into __attribute__((fallthrough)), but also provided statistics of every possible spelling of fallthrough seen in the kernel, which is useful data for those looking to fix every possible case.