| Message ID | 20161011215238.GF7282@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/11/2016 11:52 PM, Jakub Jelinek wrote: > The following patch introduces difference warning levels for > -Wimplicit-fallthrough warning, so projects can choose if they want to > honor only attributes (-Wimplicit-fallthrough=5), or what kind of comments. > =4 is very picky and accepts only very small amount of comments, =3 is what > we had before this patch, =2 looks case insensitively for falls?[ \t-]*thr(u|ough) > anywhere in the comment, =1 accepts any comment, =0 is the same as > -Wno-implicit-fallthrough - disables the warning. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? I think this is ok, and thank you very much for doing this. > The patch keeps as the default the current forms, I'm not against changing > it to =2 if there is consensus on it, but would strongly prefer doing that > incrementally, as e.g. we'll need to adjust the testsuite for that, and > perhaps also use =3 as the warning for gcc bootstraps when we are already =3 > mode clear. It's a discussion we should have, but I agree it should be done incrementally. I would argue for =1 as the default. > * c.opt (Wextra): Add as C/C++/ObjC/ObjC++ option. > (Wimplicit-fallthrough=): Enable for these languages by -Wextra. This bit looks like it does a bit more magic than is immediately obvious. Could you elaborate how this works? Bernd
On 10/12/2016 12:34 AM, Bernd Schmidt wrote: >> * c.opt (Wextra): Add as C/C++/ObjC/ObjC++ option. >> (Wimplicit-fallthrough=): Enable for these languages by -Wextra. > > This bit looks like it does a bit more magic than is immediately > obvious. Could you elaborate how this works? Ok, so it looks like these are just declarations for things defined in common.opt - but why not move Wimplicit-fallthrough over to c.opt entirely, or alternatively keep everything in common.opt as it is now? I feel like I'm missing something. Bernd
On Wed, Oct 12, 2016 at 12:34:45AM +0200, Bernd Schmidt wrote: > On 10/11/2016 11:52 PM, Jakub Jelinek wrote: > >The following patch introduces difference warning levels for > >-Wimplicit-fallthrough warning, so projects can choose if they want to > >honor only attributes (-Wimplicit-fallthrough=5), or what kind of comments. > >=4 is very picky and accepts only very small amount of comments, =3 is what > >we had before this patch, =2 looks case insensitively for falls?[ \t-]*thr(u|ough) > >anywhere in the comment, =1 accepts any comment, =0 is the same as > >-Wno-implicit-fallthrough - disables the warning. > > > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > I think this is ok, and thank you very much for doing this. > > >The patch keeps as the default the current forms, I'm not against changing > >it to =2 if there is consensus on it, but would strongly prefer doing that > >incrementally, as e.g. we'll need to adjust the testsuite for that, and > >perhaps also use =3 as the warning for gcc bootstraps when we are already =3 > >mode clear. > > It's a discussion we should have, but I agree it should be done > incrementally. I would argue for =1 as the default. > > > * c.opt (Wextra): Add as C/C++/ObjC/ObjC++ option. > > (Wimplicit-fallthrough=): Enable for these languages by -Wextra. > > This bit looks like it does a bit more magic than is immediately obvious. > Could you elaborate how this works? It is all magic to get it working correctly, took me a while to figure it out. The thing is that EnabledBy doesn't support the positive and negative arguments (and after all, -Wimplicit-fallthrough doesn't make much sense to be enabled for non-C family of languages from -Wextra), so it needs LangEnabledBy. But, LangEnabledBy will not work if the option doesn't have a corresponding language mask, so I had to add that mask to it, otherwise C_handle_option_auto, CXX_handle_option_auto etc. wouldn't be called for that option. E.g. fortran/lang.opt already had: Wcompare-reals Fortran Warning Var(warn_compare_reals) LangEnabledBy(Fortran,Wextra) Warn about equality comparisons involving REAL or COMPLEX expressions. and to be able to do that also had: Wextra Fortran Warning ; Documented in common -Wall which has far more LangEnabledBy uses is even not defined in common.opt, only in the FE *.opt files. Wunused is another option that is similarly copied/extended in c.opt, so that it can be used in LangEnabledBy and can also have its own LangEnabledBy. Jakub
On Wed, Oct 12, 2016 at 12:52:49AM +0200, Bernd Schmidt wrote: > On 10/12/2016 12:34 AM, Bernd Schmidt wrote: > > >> * c.opt (Wextra): Add as C/C++/ObjC/ObjC++ option. > >> (Wimplicit-fallthrough=): Enable for these languages by -Wextra. > > > >This bit looks like it does a bit more magic than is immediately > >obvious. Could you elaborate how this works? > > Ok, so it looks like these are just declarations for things defined in > common.opt - but why not move Wimplicit-fallthrough over to c.opt entirely, > or alternatively keep everything in common.opt as it is now? I feel like I'm > missing something. The reason to keep the option in common.opt is that we need to use the warn_* flag and OPT_W* in the middle end. Dunno if it is possible at all to configure gcc such that it builds only non-c-family cross-compiler (then it wouldn't compile because of that), but even if it is not possible it would look unclean to depend in the generic code on something from c-family/c.opt only. The extension of entries is something done for lots of options. What I perhaps should try is removing the Common keyword from the Wimplicit-fallthrough and Wimplicit-fallthrough= entries, e.g. similarly to how Wnonnull-compare is defined just as Var(...) Warning in common.opt (so that it can be used in the middle-end), but then only c-family/c.opt extends it and says the option is C C++ ObjC ObjC++. And it seems to work well for that option, ./f951 -Wnonnull-compare gives Warning: command line option ‘-Wnonnull-compare’ is valid for C/C++/ObjC/ObjC++ but not for Fortran Can I do that as a follow-up? Jakub
On 10/12/2016 01:12 AM, Jakub Jelinek wrote: > What I perhaps should try is removing the Common keyword from the > Wimplicit-fallthrough and Wimplicit-fallthrough= entries, e.g. similarly > to how Wnonnull-compare is defined just as Var(...) Warning in common.opt > (so that it can be used in the middle-end), but then only c-family/c.opt > extends it and says the option is C C++ ObjC ObjC++. > And it seems to work well for that option, ./f951 -Wnonnull-compare gives > Warning: command line option ‘-Wnonnull-compare’ is valid for C/C++/ObjC/ObjC++ but not for Fortran > > Can I do that as a follow-up? Sure. Thanks for the explanation, it's good to have that in the archive for reference. In case I wasn't clear, the patch is ok as-is. Bernd
On 2016.10.12 at 00:34 +0200, Bernd Schmidt wrote: > On 10/11/2016 11:52 PM, Jakub Jelinek wrote: > > The following patch introduces difference warning levels for > > -Wimplicit-fallthrough warning, so projects can choose if they want to > > honor only attributes (-Wimplicit-fallthrough=5), or what kind of comments. > > =4 is very picky and accepts only very small amount of comments, =3 is what > > we had before this patch, =2 looks case insensitively for falls?[ \t-]*thr(u|ough) > > anywhere in the comment, =1 accepts any comment, =0 is the same as > > -Wno-implicit-fallthrough - disables the warning. > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > I think this is ok, and thank you very much for doing this. > > > The patch keeps as the default the current forms, I'm not against changing > > it to =2 if there is consensus on it, but would strongly prefer doing that > > incrementally, as e.g. we'll need to adjust the testsuite for that, and > > perhaps also use =3 as the warning for gcc bootstraps when we are already =3 > > mode clear. > > It's a discussion we should have, but I agree it should be done > incrementally. I would argue for =1 as the default. Here are some numbers for an allmodconfig Linux kernel on pcc64le: -Wimplicit-fallthrough=1 : 951 warnings -Wimplicit-fallthrough=2 : 1087 warnings -Wimplicit-fallthrough=3 : 1209 warnings I randomly looked at the differences and almost all additional -Wimplicit-fallthrough=2 warnings are bogus (~5% are genuine). And _all_ additional -Wimplicit-fallthrough=3 warnings appear to be bogus.
On 10/12/2016 11:31 AM, Markus Trippelsdorf wrote: > On 2016.10.12 at 00:34 +0200, Bernd Schmidt wrote: >> It's a discussion we should have, but I agree it should be done >> incrementally. I would argue for =1 as the default. > > Here are some numbers for an allmodconfig Linux kernel on pcc64le: > > -Wimplicit-fallthrough=1 : 951 warnings > -Wimplicit-fallthrough=2 : 1087 warnings > -Wimplicit-fallthrough=3 : 1209 warnings > > I randomly looked at the differences and almost all additional > -Wimplicit-fallthrough=2 warnings are bogus (~5% are genuine). > And _all_ additional -Wimplicit-fallthrough=3 warnings appear > to be bogus. And that's for a codebase that was written in English to begin with. Would you mind posting one or two examples if you saw interesting ones, for reference? This result suggests that we should probably collapse levels 3-5 into a single strict one that doesn't try to be clever, and definitely make at most level 1 the default. Another thing, was it ever resolved what this warning does to tools like ccache which like to operate on preprocessed files? Bernd
On 2016.10.12 at 11:52 +0200, Bernd Schmidt wrote: > On 10/12/2016 11:31 AM, Markus Trippelsdorf wrote: > > On 2016.10.12 at 00:34 +0200, Bernd Schmidt wrote: > > > It's a discussion we should have, but I agree it should be done > > > incrementally. I would argue for =1 as the default. > > > > Here are some numbers for an allmodconfig Linux kernel on pcc64le: > > > > -Wimplicit-fallthrough=1 : 951 warnings > > -Wimplicit-fallthrough=2 : 1087 warnings > > -Wimplicit-fallthrough=3 : 1209 warnings > > > > I randomly looked at the differences and almost all additional > > -Wimplicit-fallthrough=2 warnings are bogus (~5% are genuine). > > And _all_ additional -Wimplicit-fallthrough=3 warnings appear > > to be bogus. > > And that's for a codebase that was written in English to begin with. Would > you mind posting one or two examples if you saw interesting ones, for > reference? Actually looking more closely it appears that all of the 136 additional warnings for level 2 are bogus, too. Here is an example: switch (class) { case ATA_DEV_SEMB: class = ATA_DEV_ATA; /* some hard drives report SEMB sig */ case ATA_DEV_ATA: case ATA_DEV_ZAC: tf.command = ATA_CMD_ID_ATA; break; > This result suggests that we should probably collapse levels 3-5 into a > single strict one that doesn't try to be clever, and definitely make at most > level 1 the default. Yes, I agree. -- Markus
On 10/12/2016 12:08 PM, Markus Trippelsdorf wrote: > Actually looking more closely it appears that all of the 136 additional > warnings for level 2 are bogus, too. Here is an example: > > switch (class) { > case ATA_DEV_SEMB: > class = ATA_DEV_ATA; /* some hard drives report SEMB sig */ > case ATA_DEV_ATA: > case ATA_DEV_ZAC: > tf.command = ATA_CMD_ID_ATA; > break; Another interesting question would be, how many of the level 1 warnings are false positives? But I'm not going to make you go through all 951 of them. Bernd
On Wed, Oct 12, 2016 at 11:52:02AM +0200, Bernd Schmidt wrote: > On 10/12/2016 11:31 AM, Markus Trippelsdorf wrote: > >On 2016.10.12 at 00:34 +0200, Bernd Schmidt wrote: > >>It's a discussion we should have, but I agree it should be done > >>incrementally. I would argue for =1 as the default. > > > >Here are some numbers for an allmodconfig Linux kernel on pcc64le: > > > >-Wimplicit-fallthrough=1 : 951 warnings > >-Wimplicit-fallthrough=2 : 1087 warnings > >-Wimplicit-fallthrough=3 : 1209 warnings > > > >I randomly looked at the differences and almost all additional > >-Wimplicit-fallthrough=2 warnings are bogus (~5% are genuine). > >And _all_ additional -Wimplicit-fallthrough=3 warnings appear > >to be bogus. > > And that's for a codebase that was written in English to begin with. Would > you mind posting one or two examples if you saw interesting ones, for > reference? > > This result suggests that we should probably collapse levels 3-5 into a > single strict one that doesn't try to be clever, and definitely make at most > level 1 the default. What do you mean at most? level 0 is the warning disabled, that is the default except for -Wextra. The difference between =1 and =2 is very small amount of warnings, one will need to annotate or add break; to those 951 spots anyway to make it -Wextra clean (those that don't have any kind of comment at all), so just handling the additional 136 ones as well is not that big deal. It would be interesting to see those 136 comments though, whether anything in them is about the intentional fall through or if they are just unrelated. Collapsing 3-5 levels is a bad idea, various projects will want to only rely on attributes, especially if other compilers only support attributes and nothing else, which is why there is level 5. Levels 4 and 3 give choice on how much free form the comments are. > Another thing, was it ever resolved what this warning does to tools like > ccache which like to operate on preprocessed files? We already have 2 real-world PRs about cases like: case 2: bar (); /* FALLTHRU */ #undef X case 3: #ifdef Y bar (); /* FALLTHRU */ #endif case 4: bar (); not being handled, which are extremely difficult to handle the current way in libcpp, there can be many tokens in between the fallthrough_comment_p comments and the case/default/label tokens. It should work fine with -C or -CC. So I'm wondering if a better approach wouldn't be that for -Wimplicit-fallthrough={1,2,3,4,5} we'd let the fallthrough_comment_p comments get through (perhaps normalized if not -C/-CC) as CPP_COMMENT with the PREV_FALLTHROUGH flag and perhaps also another one that would indicate it is really whitespace for other purposes. The -C/-CC description talks about significant differences though, e.g. /*FALLTHROUGH*/#define A 1 A is preprocessed differently with -C/-CC and without, so if we want to go that way, we'd also need to special case the non-C/CC added CPP_COMMENT. I have no idea what else it would affect though :(, I'm worried about token pasting and lots of other cases. If that is resolved, we could just emit the normalized /*FALLTHROUGH*/ comments if {-Wextra,-W,-Wimplicit-fallthrough{,=1,=2,=3,=4}} into -E output too and just document that we do that. Of course, for ccache I keep suggesting that they use -fdirectives-only preprocessing instead, because anything else breaks miserably tons of other stuff we have added into GCC over the last decade (-Wmisleading-indentation, -Wsystem-headers vs. macro contexts, etc.), but the maintainers disagree. Jakub
On 10/12/2016 12:26 PM, Jakub Jelinek wrote: > What do you mean at most? level 0 is the warning disabled, that is the > default except for -Wextra. I think level 0 has to be on the table for -Wextra as well, depending on the signal-to-noise ratio of level 1. > Collapsing 3-5 levels is a bad idea, various projects will want to only > rely on attributes, especially if other compilers only support attributes > and nothing else, which is why there is level 5. Levels 4 and 3 give choice > on how much free form the comments are. But I think the data we have points towards that being a meaningless choice. So maybe I got the levels wrong and 2-4 is what we should be collapsing. >> Another thing, was it ever resolved what this warning does to tools like >> ccache which like to operate on preprocessed files? > > We already have 2 real-world PRs about cases like: Yeah. Another reason why I think level 0 (i.e. off) should be seriously considered for -Wextra. Bernd
On Wed, Oct 12, 2016 at 12:08:40PM +0200, Markus Trippelsdorf wrote: > On 2016.10.12 at 11:52 +0200, Bernd Schmidt wrote: > > On 10/12/2016 11:31 AM, Markus Trippelsdorf wrote: > > > On 2016.10.12 at 00:34 +0200, Bernd Schmidt wrote: > > > > It's a discussion we should have, but I agree it should be done > > > > incrementally. I would argue for =1 as the default. > > > > > > Here are some numbers for an allmodconfig Linux kernel on pcc64le: > > > > > > -Wimplicit-fallthrough=1 : 951 warnings > > > -Wimplicit-fallthrough=2 : 1087 warnings > > > -Wimplicit-fallthrough=3 : 1209 warnings > > > > > > I randomly looked at the differences and almost all additional > > > -Wimplicit-fallthrough=2 warnings are bogus (~5% are genuine). > > > And _all_ additional -Wimplicit-fallthrough=3 warnings appear > > > to be bogus. > > > > And that's for a codebase that was written in English to begin with. Would > > you mind posting one or two examples if you saw interesting ones, for > > reference? > > Actually looking more closely it appears that all of the 136 additional > warnings for level 2 are bogus, too. Here is an example: > > switch (class) { > case ATA_DEV_SEMB: > class = ATA_DEV_ATA; /* some hard drives report SEMB sig */ > case ATA_DEV_ATA: > case ATA_DEV_ZAC: > tf.command = ATA_CMD_ID_ATA; > break; This is a clear argument for -Wimplicit-fallthrough default to =2 or higher. The comment is completely random, doesn't express in any way the intent to fall through, and if the comment appeared one line earlier (just slightly different coding style), we'd warn. I don't see a difference between why we shouldn't warn by default (still talking about -Wextra) for the above and should for: switch (class) { case ATA_DEV_SEMB: class = ATA_DEV_ATA; case ATA_DEV_ATA: case ATA_DEV_ZAC: tf.command = ATA_CMD_ID_ATA; break; In neither case the warning is bogus, it isn't even clear without additional code analysis whether it is a code bug or intentational fallthrough not marked that way. The reason for adding =1 mode has been to support projects that can't be bothered to express their intent in any standardized way and want to express it free form in arbitrary languages, using various wordings etc. At the expense of missing cases like the above, where they actually don't express the intent in any way. Jakub
On 2016.10.12 at 12:26 +0200, Jakub Jelinek wrote: > On Wed, Oct 12, 2016 at 11:52:02AM +0200, Bernd Schmidt wrote: > > On 10/12/2016 11:31 AM, Markus Trippelsdorf wrote: > > >On 2016.10.12 at 00:34 +0200, Bernd Schmidt wrote: > > >>It's a discussion we should have, but I agree it should be done > > >>incrementally. I would argue for =1 as the default. > > > > > >Here are some numbers for an allmodconfig Linux kernel on pcc64le: > > > > > >-Wimplicit-fallthrough=1 : 951 warnings > > >-Wimplicit-fallthrough=2 : 1087 warnings > > >-Wimplicit-fallthrough=3 : 1209 warnings > > > > > >I randomly looked at the differences and almost all additional > > >-Wimplicit-fallthrough=2 warnings are bogus (~5% are genuine). > > >And _all_ additional -Wimplicit-fallthrough=3 warnings appear > > >to be bogus. > > > > And that's for a codebase that was written in English to begin with. Would > > you mind posting one or two examples if you saw interesting ones, for > > reference? > > > > This result suggests that we should probably collapse levels 3-5 into a > > single strict one that doesn't try to be clever, and definitely make at most > > level 1 the default. > > What do you mean at most? level 0 is the warning disabled, that is the > default except for -Wextra. The difference between =1 and =2 is very small > amount of warnings, one will need to annotate or add break; to those 951 > spots anyway to make it -Wextra clean (those that don't have any kind of > comment at all), so just handling the additional 136 ones as well is not > that big deal. It would be interesting to see those 136 comments though, > whether anything in them is about the intentional fall through or if they > are just unrelated. Here are some examples: /* Don't break */ /* drop through */ //very popular /* fall though */ /* pass through... */ /* break intentionally omitted */ /* fallthough */ /* Don't break, this is a failed frame! */ /* leave break out intentionly */ /* else: Fall trough */ /* passthrough */ /* NOBREAK */ /* follow thru */ /* go through here */
On 10/12/2016 12:36 PM, Markus Trippelsdorf wrote: > Here are some examples: > > /* Don't break */ > /* drop through */ //very popular > /* fall though */ > /* pass through... */ > /* break intentionally omitted */ > /* fallthough */ > /* Don't break, this is a failed frame! */ > /* leave break out intentionly */ > /* else: Fall trough */ > /* passthrough */ > /* NOBREAK */ > /* follow thru */ > /* go through here */ Yeah. As I said earlier, this is a losing battle without real AI, even when we're only considering English as the comment language. Bernd
On Wed, Oct 12, 2016 at 12:33:58PM +0200, Bernd Schmidt wrote: > On 10/12/2016 12:26 PM, Jakub Jelinek wrote: > >What do you mean at most? level 0 is the warning disabled, that is the > >default except for -Wextra. > > I think level 0 has to be on the table for -Wextra as well, depending on the > signal-to-noise ratio of level 1. -Wextra includes various style warnings (we have some of them even in -Wall), and the intent with this warning is to improve the code base, let users look at places where there are implicit fallthrough not explicitly marked so, and spend the few seconds to mark intentional fallthrough if it is indeed intentional. So for this warning, the question is what is a false positive; I really don't expect the code in the wild broken so much that every place the warning is emitted is a missing break;, 1:50 or 1:100 ratio is just fine, by adding (more or less free form, which is why we have levels) comments about it you express the intent and that will survive often for a long time without need for another analysis. If -Wimplicit-fallthrough isn't included in -Wextra, most of the people will not even know there is this warning, and the code base in the wild will remain with lots of bugs. Remember how many bugs in gcc sources alone this warning allowed to fix. Jakub
On Wed, Oct 12, 2016 at 12:36:29PM +0200, Markus Trippelsdorf wrote: > Here are some examples: > > /* Don't break */ > /* drop through */ //very popular > /* fall though */ > /* pass through... */ > /* break intentionally omitted */ > /* fallthough */ > /* Don't break, this is a failed frame! */ > /* leave break out intentionly */ > /* else: Fall trough */ > /* passthrough */ > /* NOBREAK */ > /* follow thru */ > /* go through here */ It would be nice to see all those 136 comments, perhaps sort | uniq -c merged together. Letting people fix fall though or fallthough or Fall trough misspellings is a good thing. And for the others, I think the comment adjustment won't take more than a few seconds each, compared to having to analyze the 900+ cases. Jakub
On 2016.10.12 at 12:54 +0200, Jakub Jelinek wrote: > On Wed, Oct 12, 2016 at 12:36:29PM +0200, Markus Trippelsdorf wrote: > > Here are some examples: > > > > /* Don't break */ > > /* drop through */ //very popular > > /* fall though */ > > /* pass through... */ > > /* break intentionally omitted */ > > /* fallthough */ > > /* Don't break, this is a failed frame! */ > > /* leave break out intentionly */ > > /* else: Fall trough */ > > /* passthrough */ > > /* NOBREAK */ > > /* follow thru */ > > /* go through here */ > > It would be nice to see all those 136 comments, perhaps sort | uniq -c > merged together. > Letting people fix fall though or fallthough or Fall trough misspellings > is a good thing. And for the others, I think the comment adjustment won't > take more than a few seconds each, compared to having to analyze the 900+ > cases. I'm more concerned about the first impression that people will get from this warning. If the fist couple of samples they will look at are clearly bogus (as with the example comments above) they will very quickly disable the warning. And if a small one digit percentage of all potential issues falls through the cracks with -Wimplicit-fallthrough=1 , so be it.
On 10/12/2016 01:04 PM, Markus Trippelsdorf wrote: > I'm more concerned about the first impression that people will get from > this warning. If the fist couple of samples they will look at are > clearly bogus (as with the example comments above) they will very > quickly disable the warning. > > And if a small one digit percentage of all potential issues falls > through the cracks with -Wimplicit-fallthrough=1 , so be it. Yes. Better to give mostly sensible warnings, making people aware of the option and maybe encouraging to try higher levels, rather than generating a huge amount of noise which will result in some people turning the warning off, some making pointles or even incorrect changes to their code (as we saw in gcc itself), and generating bad publicity. Bernd
On Wed, Oct 12, 2016 at 04:04:25PM +0200, Bernd Schmidt wrote: > On 10/12/2016 01:04 PM, Markus Trippelsdorf wrote: > >I'm more concerned about the first impression that people will get from > >this warning. If the fist couple of samples they will look at are > >clearly bogus (as with the example comments above) they will very > >quickly disable the warning. > > > >And if a small one digit percentage of all potential issues falls > >through the cracks with -Wimplicit-fallthrough=1 , so be it. > > Yes. Better to give mostly sensible warnings, making people aware of the > option and maybe encouraging to try higher levels, rather than generating a > huge amount of noise which will result in some people turning the warning > off, some making pointles or even incorrect changes to their code (as we saw > in gcc itself), and generating bad publicity. But whatever non-zero level it is, the most common case in most projects will be not the intentional fallthrus marked with custom style comments, but those not marked at all. E.g. in the linux kernel case, that is 90% of the warnings with -Wimplicit-fallthrough=2. Whether you tweak another 10% isn't something that makes significant difference. What incorrect changes to code in gcc do you have in mind? I just remember one spot where a /* FALLTHRU */ comment has been added even when it should be break;, that is no code change. Most of the cases in gcc that had to change were where there were no fallthru comments at all, not where a custom style comment has been changed to some more standardized one. Jakub
On Tuesday 11 October 2016, Jakub Jelinek wrote: > Hi! > > The following patch introduces difference warning levels for > -Wimplicit-fallthrough warning, so projects can choose if they want to > honor only attributes (-Wimplicit-fallthrough=5), or what kind of comments. > =4 is very picky and accepts only very small amount of comments, =3 is what > we had before this patch, =2 looks case insensitively for falls?[ > \t-]*thr(u|ough) anywhere in the comment, =1 accepts any comment, =0 is > the same as -Wno-implicit-fallthrough - disables the warning. I would suggest also looking for comments with "no break" in them, as that is another common way to annotate the intentional lack of a 'break'. If you want another example besides the linux kernel, I unified all our fall through comments in qtbase in August: https://codereview.qt-project.org/#/c/163595/ Though Qt is far from -Wimpliciit-fallthough clean after that, another colleague is working on that since we have traditionally aimed for -Wall - Wextra -Werror, though it will seriously wreck hawock with readability in several places with unrolled loops and switches on integers. Best regards `Allan
--- gcc/common.opt.jj 2016-10-01 00:44:48.000000000 +0200 +++ gcc/common.opt 2016-10-11 17:57:15.361670576 +0200 @@ -602,7 +602,10 @@ Common Var(warn_hsa) Init(1) Warning Warn when a function cannot be expanded to HSAIL. Wimplicit-fallthrough -Common Var(warn_implicit_fallthrough) Warning EnabledBy(Wextra) +Common Alias(Wimplicit-fallthrough=,3,0) Warning + +Wimplicit-fallthrough= +Common Var(warn_implicit_fallthrough) RejectNegative Joined UInteger Warning Warn when a switch case falls through. Winline --- gcc/gimplify.c.jj 2016-10-09 13:19:09.000000000 +0200 +++ gcc/gimplify.c 2016-10-11 17:29:13.280800199 +0200 @@ -1918,7 +1918,7 @@ warn_implicit_fallthrough_r (gimple_stmt else if (gimple_code (prev) == GIMPLE_LABEL && (label = gimple_label_label (as_a <glabel *> (prev))) && (l = find_label_entry (&labels, label))) - warned_p = warning_at (l->loc, OPT_Wimplicit_fallthrough, + warned_p = warning_at (l->loc, OPT_Wimplicit_fallthrough_, "this statement may fall through"); else if (!gimple_call_internal_p (prev, IFN_FALLTHROUGH) /* Try to be clever and don't warn when the statement @@ -1926,7 +1926,7 @@ warn_implicit_fallthrough_r (gimple_stmt && gimple_stmt_may_fallthru (prev) && gimple_has_location (prev)) warned_p = warning_at (gimple_location (prev), - OPT_Wimplicit_fallthrough, + OPT_Wimplicit_fallthrough_, "this statement may fall through"); if (warned_p) inform (gimple_location (next), "here"); --- gcc/doc/invoke.texi.jj 2016-10-09 13:17:42.000000000 +0200 +++ gcc/doc/invoke.texi 2016-10-11 20:19:33.322117754 +0200 @@ -273,8 +273,8 @@ Objective-C and Objective-C++ Dialects}. -Wformat-security -Wformat-signedness -Wformat-y2k -Wframe-address @gol -Wframe-larger-than=@var{len} -Wno-free-nonheap-object -Wjump-misses-init @gol -Wignored-qualifiers -Wignored-attributes -Wincompatible-pointer-types @gol --Wimplicit -Wimplicit-fallthrough -Wimplicit-function-declaration @gol --Wimplicit-int @gol +-Wimplicit -Wimplicit-fallthrough -Wimplicit-fallthrough=@var{n} @gol +-Wimplicit-function-declaration -Wimplicit-int @gol -Winit-self -Winline -Wno-int-conversion -Wint-in-bool-context @gol -Wno-int-to-pointer-cast -Winvalid-memory-model -Wno-invalid-offsetof @gol -Winvalid-pch -Wlarger-than=@var{len} @gol @@ -3734,7 +3734,7 @@ name is still supported, but the newer n @gccoptlist{-Wclobbered @gol -Wempty-body @gol -Wignored-qualifiers @gol --Wimplicit-fallthrough @gol +-Wimplicit-fallthrough=3 @gol -Wmissing-field-initializers @gol -Wmissing-parameter-type @r{(C only)} @gol -Wold-style-declaration @r{(C only)} @gol @@ -4106,6 +4106,12 @@ This warning is enabled by @option{-Wall @item -Wimplicit-fallthrough @opindex Wimplicit-fallthrough @opindex Wno-implicit-fallthrough +@option{-Wimplicit-fallthrough} is the same as @option{-Wimplicit-fallthrough=3} +and @option{-Wno-implicit-fallthrough} is the same as +@option{-Wimplicit-fallthrough=0}. + +@item -Wimplicit-fallthrough=@var{n} +@opindex Wimplicit-fallthrough= Warn when a switch case falls through. For example: @smallexample @@ -4126,7 +4132,7 @@ switch (cond) This warning does not warn when the last statement of a case cannot fall through, e.g. when there is a return statement or a call to function -declared with the noreturn attribute. @option{-Wimplicit-fallthrough} +declared with the noreturn attribute. @option{-Wimplicit-fallthrough=} also takes into account control flow statements, such as ifs, and only warns when appropriate. E.g.@: @@ -4169,9 +4175,23 @@ switch (cond) C++17 provides a standard way to suppress the @option{-Wimplicit-fallthrough} warning using @code{[[fallthrough]];} instead of the GNU attribute. In C++11 or C++14 users can use @code{[[gnu::fallthrough]];}, which is a GNU extension. -Instead of the these attributes, it is also possible to add a "falls through" +Instead of the these attributes, it is also possible to add a fallthrough comment to silence the warning. The whole body of the C or C++ style comment -should match one of the following regular expressions: +should match the given regular expressions listed below. The option argument +@var{n} specifies what kind of comments are accepted: + +@itemize @bullet + +@item @option{-Wimplicit-fallthrough=0} disables the warning altogether. + +@item @option{-Wimplicit-fallthrough=1} matches @code{.*} regular +expression, any comment is used as fallthrough comment. + +@item @option{-Wimplicit-fallthrough=2} case insensitively matches +@code{.*falls?[ \t-]*thr(ough|u).*} regular expression. + +@item @option{-Wimplicit-fallthrough=3} case sensitively matches one of the +following regular expressions: @itemize @bullet @@ -4179,17 +4199,37 @@ should match one of the following regula @item @code{@@fallthrough@@} -@item @code{lint -fallthrough ?} +@item @code{lint -fallthrough[ \t]*} + +@item @code{[ \t.!]*(ELSE,? |INTENTIONAL(LY)? )?@*FALL(S | |-)?THR(OUGH|U)[ \t.!]*(-[^\n\r]*)?} + +@item @code{[ \t.!]*(Else,? |Intentional(ly)? )?@*Fall((s | |-)[Tt]|t)hr(ough|u)[ \t.!]*(-[^\n\r]*)?} -@item @code{[ \t.!]*(ELSE,? |INTENTIONAL(LY)? )?FALL(S | |-)?THR(OUGH|U)[ \t.!]*(-[^\n\r]*)?} +@item @code{[ \t.!]*([Ee]lse,? |[Ii]ntentional(ly)? )?@*fall(s | |-)?thr(ough|u)[ \t.!]*(-[^\n\r]*)?} + +@end itemize -@item @code{[ \t.!]*(Else,? |Intentional(ly)? )?Fall((s | |-)[Tt]|t)hr(ough|u)[ \t.!]*(-[^\n\r]*)?} +@item @option{-Wimplicit-fallthrough=4} case sensitively matches one of the +following regular expressions: + +@itemize @bullet + +@item @code{-fallthrough} + +@item @code{@@fallthrough@@} + +@item @code{lint -fallthrough[ \t]*} + +@item @code{[ \t]*FALLTHR(OUGH|U)[ \t]*} + +@end itemize -@item @code{[ \t.!]*([Ee]lse,? |[Ii]ntentional(ly)? )?fall(s | |-)?thr(ough|u)[ \t.!]*(-[^\n\r]*)?} +@item @option{-Wimplicit-fallthrough=5} doesn't recognize any comments as +fallthrough comments, only attributes disable the warning. @end itemize -and the comment needs to be followed after optional whitespace and other comments +The comment needs to be followed after optional whitespace and other comments by @code{case} or @code{default} keywords or by a user label that preceeds some @code{case} or @code{default} label. @@ -4206,7 +4246,7 @@ switch (cond) @end group @end smallexample -This warning is enabled by @option{-Wextra}. +The @option{-Wimplicit-fallthrough=3} warning is enabled by @option{-Wextra}. @item -Wignored-qualifiers @r{(C and C++ only)} @opindex Wignored-qualifiers --- gcc/c-family/c.opt.jj 2016-09-29 22:53:53.000000000 +0200 +++ gcc/c-family/c.opt 2016-10-11 19:03:50.316364851 +0200 @@ -462,6 +462,10 @@ Werror-implicit-function-declaration C ObjC RejectNegative Warning Alias(Werror=, implicit-function-declaration) This switch is deprecated; use -Werror=implicit-function-declaration instead. +Wextra +C ObjC C++ ObjC++ Warning +; in common.opt + Wfloat-conversion C ObjC C++ ObjC++ Var(warn_float_conversion) Warning LangEnabledBy(C ObjC C++ ObjC++,Wconversion) Warn for implicit type conversions that cause loss of floating point precision. @@ -536,6 +540,10 @@ Wimplicit C ObjC Var(warn_implicit) Warning LangEnabledBy(C ObjC,Wall) Warn about implicit declarations. +Wimplicit-fallthrough= +LangEnabledBy(C ObjC C++ ObjC++,Wextra,3,0) +; in common.opt + Wdouble-promotion C ObjC C++ ObjC++ Var(warn_double_promotion) Warning Warn about implicit conversions from \"float\" to \"double\". --- gcc/c-family/c-opts.c.jj 2016-09-29 22:53:53.000000000 +0200 +++ gcc/c-family/c-opts.c 2016-10-11 17:17:30.150632637 +0200 @@ -1279,6 +1279,11 @@ sanitize_cpp_opts (void) if (flag_working_directory == -1) flag_working_directory = (debug_info_level != DINFO_LEVEL_NONE); + if (warn_implicit_fallthrough < 5) + cpp_opts->cpp_warn_implicit_fallthrough = warn_implicit_fallthrough; + else + cpp_opts->cpp_warn_implicit_fallthrough = 0; + if (cpp_opts->directives_only) { if (cpp_warn_unused_macros) --- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-25.c.jj 2016-10-11 18:35:49.430545561 +0200 +++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-25.c 2016-10-11 18:37:14.325475034 +0200 @@ -0,0 +1,33 @@ +/* { dg-do compile } */ +/* { dg-options "-Wextra" } */ + +void bar (int); + +void +foo (int i) +{ + switch (i) + { + case 1: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (1); + case 2: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (2); + /* Some comment. */ + case 3: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (3); + /* Here we really do want to fALl tHRoUgh and we mean it! */ + case 4: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (4); + /* Intentionally fall through. */ + case 5: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (5); + /* FALLTHROUGH */ + case 6: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (6); + __attribute__((fallthrough)); + case 7: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (7); + default: + break; + } +} --- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-26.c.jj 2016-10-11 18:38:55.087204425 +0200 +++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-26.c 2016-10-11 18:37:30.973265104 +0200 @@ -0,0 +1,33 @@ +/* { dg-do compile } */ +/* { dg-options "-Wimplicit-fallthrough" } */ + +void bar (int); + +void +foo (int i) +{ + switch (i) + { + case 1: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (1); + case 2: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (2); + /* Some comment. */ + case 3: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (3); + /* Here we really do want to fALl tHRoUgh and we mean it! */ + case 4: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (4); + /* Intentionally fall through. */ + case 5: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (5); + /* FALLTHROUGH */ + case 6: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (6); + __attribute__((fallthrough)); + case 7: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (7); + default: + break; + } +} --- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-27.c.jj 2016-10-11 18:38:58.172165524 +0200 +++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-27.c 2016-10-11 18:38:08.283794618 +0200 @@ -0,0 +1,33 @@ +/* { dg-do compile } */ +/* { dg-options "-Wimplicit-fallthrough=1" } */ + +void bar (int); + +void +foo (int i) +{ + switch (i) + { + case 1: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (1); + case 2: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (2); + /* Some comment. */ + case 3: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (3); + /* Here we really do want to fALl tHRoUgh and we mean it! */ + case 4: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (4); + /* Intentionally fall through. */ + case 5: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (5); + /* FALLTHROUGH */ + case 6: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (6); + __attribute__((fallthrough)); + case 7: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (7); + default: + break; + } +} --- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-28.c.jj 2016-10-11 18:39:01.235126900 +0200 +++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-28.c 2016-10-11 18:38:50.014268395 +0200 @@ -0,0 +1,33 @@ +/* { dg-do compile } */ +/* { dg-options "-Wimplicit-fallthrough=2" } */ + +void bar (int); + +void +foo (int i) +{ + switch (i) + { + case 1: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (1); + case 2: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (2); + /* Some comment. */ + case 3: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (3); + /* Here we really do want to fALl tHRoUgh and we mean it! */ + case 4: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (4); + /* Intentionally fall through. */ + case 5: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (5); + /* FALLTHROUGH */ + case 6: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (6); + __attribute__((fallthrough)); + case 7: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (7); + default: + break; + } +} --- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-29.c.jj 2016-10-11 18:39:57.641415615 +0200 +++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-29.c 2016-10-11 18:40:03.191345630 +0200 @@ -0,0 +1,33 @@ +/* { dg-do compile } */ +/* { dg-options "-Wimplicit-fallthrough=3" } */ + +void bar (int); + +void +foo (int i) +{ + switch (i) + { + case 1: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (1); + case 2: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (2); + /* Some comment. */ + case 3: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (3); + /* Here we really do want to fALl tHRoUgh and we mean it! */ + case 4: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (4); + /* Intentionally fall through. */ + case 5: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (5); + /* FALLTHROUGH */ + case 6: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (6); + __attribute__((fallthrough)); + case 7: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (7); + default: + break; + } +} --- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-30.c.jj 2016-10-11 18:40:13.065221120 +0200 +++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-30.c 2016-10-11 18:40:34.762947511 +0200 @@ -0,0 +1,33 @@ +/* { dg-do compile } */ +/* { dg-options "-Wimplicit-fallthrough=4" } */ + +void bar (int); + +void +foo (int i) +{ + switch (i) + { + case 1: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (1); + case 2: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (2); + /* Some comment. */ + case 3: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (3); + /* Here we really do want to fALl tHRoUgh and we mean it! */ + case 4: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (4); + /* Intentionally fall through. */ + case 5: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (5); + /* FALLTHROUGH */ + case 6: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (6); + __attribute__((fallthrough)); + case 7: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (7); + default: + break; + } +} --- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-31.c.jj 2016-10-11 18:40:43.659835321 +0200 +++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-31.c 2016-10-11 18:41:04.397573818 +0200 @@ -0,0 +1,33 @@ +/* { dg-do compile } */ +/* { dg-options "-Wimplicit-fallthrough=5" } */ + +void bar (int); + +void +foo (int i) +{ + switch (i) + { + case 1: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (1); + case 2: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (2); + /* Some comment. */ + case 3: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (3); + /* Here we really do want to fALl tHRoUgh and we mean it! */ + case 4: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (4); + /* Intentionally fall through. */ + case 5: /* { dg-warning "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (5); + /* FALLTHROUGH */ + case 6: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (6); + __attribute__((fallthrough)); + case 7: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (7); + default: + break; + } +} --- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-32.c.jj 2016-10-11 19:05:56.158779524 +0200 +++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-32.c 2016-10-11 19:06:11.438587033 +0200 @@ -0,0 +1,33 @@ +/* { dg-do compile } */ +/* { dg-options "-Wimplicit-fallthrough=0" } */ + +void bar (int); + +void +foo (int i) +{ + switch (i) + { + case 1: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (1); + case 2: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (2); + /* Some comment. */ + case 3: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (3); + /* Here we really do want to fALl tHRoUgh and we mean it! */ + case 4: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (4); + /* Intentionally fall through. */ + case 5: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (5); + /* FALLTHROUGH */ + case 6: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (6); + __attribute__((fallthrough)); + case 7: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (7); + default: + break; + } +} --- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-33.c.jj 2016-10-11 19:06:26.541396772 +0200 +++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-33.c 2016-10-11 19:06:33.660307090 +0200 @@ -0,0 +1,33 @@ +/* { dg-do compile } */ +/* { dg-options "-Wno-implicit-fallthrough" } */ + +void bar (int); + +void +foo (int i) +{ + switch (i) + { + case 1: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (1); + case 2: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (2); + /* Some comment. */ + case 3: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (3); + /* Here we really do want to fALl tHRoUgh and we mean it! */ + case 4: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (4); + /* Intentionally fall through. */ + case 5: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (5); + /* FALLTHROUGH */ + case 6: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (6); + __attribute__((fallthrough)); + case 7: /* { dg-bogus "this statement may \[fla\]* through" "" { target *-*-* } .+1 } */ + bar (7); + default: + break; + } +} --- libcpp/include/cpplib.h.jj 2016-09-26 12:06:48.000000000 +0200 +++ libcpp/include/cpplib.h 2016-10-11 17:17:09.641890203 +0200 @@ -395,6 +395,9 @@ struct cpp_options explicitly undefined. */ unsigned char warn_builtin_macro_redefined; + /* Different -Wimplicit-fallthrough= levels. */ + unsigned char cpp_warn_implicit_fallthrough; + /* Nonzero means we should look for header.gcc files that remap file names. */ unsigned char remap; --- libcpp/init.c.jj 2016-06-24 12:59:32.000000000 +0200 +++ libcpp/init.c 2016-10-11 17:16:57.423043538 +0200 @@ -189,6 +189,7 @@ cpp_create_reader (enum c_lang lang, cpp CPP_OPTION (pfile, warn_dollars) = 1; CPP_OPTION (pfile, warn_variadic_macros) = 1; CPP_OPTION (pfile, warn_builtin_macro_redefined) = 1; + CPP_OPTION (pfile, cpp_warn_implicit_fallthrough) = 0; /* By default, track locations of tokens resulting from macro expansion. The '2' means, track the locations with the highest accuracy. Read the comments for struct --- libcpp/lex.c.jj 2016-10-08 12:53:25.000000000 +0200 +++ libcpp/lex.c 2016-10-11 17:17:05.726939332 +0200 @@ -2040,6 +2040,64 @@ static bool fallthrough_comment_p (cpp_reader *pfile, const unsigned char *comment_start) { const unsigned char *from = comment_start + 1; + + switch (CPP_OPTION (pfile, cpp_warn_implicit_fallthrough)) + { + /* For both -Wimplicit-fallthrough=0 and -Wimplicit-fallthrough=5 we + don't recognize any comments. The latter only checks attributes, + the former doesn't warn. */ + case 0: + default: + return false; + /* -Wimplicit-fallthrough=1 considers any comment, no matter what + content it has. */ + case 1: + return true; + case 2: + /* -Wimplicit-fallthrough=2 looks for (case insensitive) + .*falls?[ \t-]*thr(u|ough).* regex. */ + for (; (size_t) (pfile->buffer->cur - from) >= sizeof "fallthru" - 1; + from++) + { + /* Is there anything like strpbrk with upper boundary, or + memchr looking for 2 characters rather than just one? */ + if (from[0] != 'f' && from[0] != 'F') + continue; + if (from[1] != 'a' && from[1] != 'A') + continue; + if (from[2] != 'l' && from[2] != 'L') + continue; + if (from[3] != 'l' && from[3] != 'L') + continue; + from += sizeof "fall" - 1; + if (from[0] == 's' || from[0] == 'S') + from++; + while (*from == ' ' || *from == '\t' || *from == '-') + from++; + if (from[0] != 't' && from[0] != 'T') + continue; + if (from[1] != 'h' && from[1] != 'H') + continue; + if (from[2] != 'r' && from[2] != 'R') + continue; + if (from[3] == 'u' || from[3] == 'U') + return true; + if (from[3] != 'o' && from[3] != 'O') + continue; + if (from[4] != 'u' && from[4] != 'U') + continue; + if (from[5] != 'g' && from[5] != 'G') + continue; + if (from[6] != 'h' && from[6] != 'H') + continue; + return true; + } + return false; + case 3: + case 4: + break; + } + /* Whole comment contents: -fallthrough @fallthrough@ @@ -2060,7 +2118,7 @@ fallthrough_comment_p (cpp_reader *pfile from += 1 + len; } /* Whole comment contents (regex): - lint -fallthrough ? + lint -fallthrough[ \t]* */ else if (*from == 'l') { @@ -2068,10 +2126,33 @@ fallthrough_comment_p (cpp_reader *pfile if ((size_t) (pfile->buffer->cur - from - 1) < len) return false; if (memcmp (from + 1, "int -fallthrough", len)) - return false; + return false; from += 1 + len; - if (*from == ' ') - from++; + while (*from == ' ' || *from == '\t') + from++; + } + /* Whole comment contents (regex): + [ \t]*FALLTHR(U|OUGH)[ \t]* + */ + else if (CPP_OPTION (pfile, cpp_warn_implicit_fallthrough) == 4) + { + while (*from == ' ' || *from == '\t') + from++; + if ((size_t) (pfile->buffer->cur - from) < sizeof "FALLTHRU" - 1) + return false; + if (memcmp (from, "FALLTHR", sizeof "FALLTHR" - 1)) + return false; + from += sizeof "FALLTHR" - 1; + if (*from == 'U') + from++; + else if ((size_t) (pfile->buffer->cur - from) < sizeof "OUGH" - 1) + return false; + else if (memcmp (from, "OUGH", sizeof "OUGH" - 1)) + return false; + else + from += sizeof "OUGH" - 1; + while (*from == ' ' || *from == '\t') + from++; } /* Whole comment contents (regex): [ \t.!]*(ELSE,? |INTENTIONAL(LY)? )?FALL(S | |-)?THR(OUGH|U)[ \t.!]*(-[^\n\r]*)? @@ -2085,8 +2166,8 @@ fallthrough_comment_p (cpp_reader *pfile unsigned char f = *from; bool all_upper = false; if (f == 'E' || f == 'e') - { - if ((size_t) (pfile->buffer->cur - from) + { + if ((size_t) (pfile->buffer->cur - from) < sizeof "else fallthru" - 1) return false; if (f == 'E' && memcmp (from + 1, "LSE", sizeof "LSE" - 1) == 0) @@ -2096,7 +2177,7 @@ fallthrough_comment_p (cpp_reader *pfile from += sizeof "else" - 1; if (*from == ',') from++; - if (*from != ' ') + if (*from != ' ') return false; from++; if (all_upper && *from == 'f') @@ -2104,10 +2185,10 @@ fallthrough_comment_p (cpp_reader *pfile if (f == 'e' && *from == 'F') return false; f = *from; - } + } else if (f == 'I' || f == 'i') - { - if ((size_t) (pfile->buffer->cur - from) + { + if ((size_t) (pfile->buffer->cur - from) < sizeof "intentional fallthru" - 1) return false; if (f == 'I' && memcmp (from + 1, "NTENTIONAL", @@ -2138,7 +2219,7 @@ fallthrough_comment_p (cpp_reader *pfile if (f == 'i' && *from == 'F') return false; f = *from; - } + } if (f != 'F' && f != 'f') return false; if ((size_t) (pfile->buffer->cur - from) < sizeof "fallthru" - 1)