Re: Change default level for -Wimplicit-fallthrough
On 11/03/2016 05:51 AM, Bernd Schmidt wrote: I'm concerned about the number of false positives for this warning, and judging by previous discussions, I'm not alone in this. This patch limits it to level 1 (any comment before the case label disables the warning) for cases where the user specified no explicit level. It'll still generate enough noise that people will be aware of it and can choose whether to use a higher level or not. Bootstrapped and tested on x86_64-linux. Ok? I think we should wait for the distro builds to fire up and see how the warning impacts them before making a decision on this patch. jeff
Re: Change default level for -Wimplicit-fallthrough
On 2016.11.03 at 14:57 +0100, Jakub Jelinek wrote: > On Thu, Nov 03, 2016 at 02:55:03PM +0100, Markus Trippelsdorf wrote: > > On 2016.11.03 at 14:47 +0100, Jakub Jelinek wrote: > > > On Thu, Nov 03, 2016 at 02:35:57PM +0100, Markus Trippelsdorf wrote: > > > > I don't have gathered detailed statistics. But for example a simple > > > > /* drop through */ in a package header file will of course cause many > > > > bogus warnings during the build on level 2. > > > > For the Linux kernel false positives decrease ~20% when switching from > > > > level 3 to 1. > > > > > > One would have to count only warnings with unique locus (i.e. sort -u them > > > after grepping them from logs). > > > But even with 20%, if one spends the energy to analyze the 80%, where > > > one actually has to analyze the code, just mechanically changing a couple > > > of > > > common comment kinds into more standardized one isn't going to be > > > significant. > > > > I should have written: For the Linux kernel the number of warnings > > dropped by 20% (going from level 3 to 1) and all of them turned out to > > be false positives. And yes, I have used "sort -u". > > I'm not sure if I would call 20% insignificant. > > But we are talking about 2 vs. 1 now, so that is likely smaller than 20%. > Plus what those comments in that 2 vs. 1 set are where the warnings differ, > if they are related to fall through or not. It is still a 12% reduction (2 vs. 1). I've posted a list of them in the older thread. -- Markus
Re: Change default level for -Wimplicit-fallthrough
On Thu, Nov 03, 2016 at 02:55:03PM +0100, Markus Trippelsdorf wrote: > On 2016.11.03 at 14:47 +0100, Jakub Jelinek wrote: > > On Thu, Nov 03, 2016 at 02:35:57PM +0100, Markus Trippelsdorf wrote: > > > I don't have gathered detailed statistics. But for example a simple > > > /* drop through */ in a package header file will of course cause many > > > bogus warnings during the build on level 2. > > > For the Linux kernel false positives decrease ~20% when switching from > > > level 3 to 1. > > > > One would have to count only warnings with unique locus (i.e. sort -u them > > after grepping them from logs). > > But even with 20%, if one spends the energy to analyze the 80%, where > > one actually has to analyze the code, just mechanically changing a couple of > > common comment kinds into more standardized one isn't going to be > > significant. > > I should have written: For the Linux kernel the number of warnings > dropped by 20% (going from level 3 to 1) and all of them turned out to > be false positives. And yes, I have used "sort -u". > I'm not sure if I would call 20% insignificant. But we are talking about 2 vs. 1 now, so that is likely smaller than 20%. Plus what those comments in that 2 vs. 1 set are where the warnings differ, if they are related to fall through or not. Jakub
Re: Change default level for -Wimplicit-fallthrough
On 2016.11.03 at 14:47 +0100, Jakub Jelinek wrote: > On Thu, Nov 03, 2016 at 02:35:57PM +0100, Markus Trippelsdorf wrote: > > I don't have gathered detailed statistics. But for example a simple > > /* drop through */ in a package header file will of course cause many > > bogus warnings during the build on level 2. > > For the Linux kernel false positives decrease ~20% when switching from > > level 3 to 1. > > One would have to count only warnings with unique locus (i.e. sort -u them > after grepping them from logs). > But even with 20%, if one spends the energy to analyze the 80%, where > one actually has to analyze the code, just mechanically changing a couple of > common comment kinds into more standardized one isn't going to be > significant. I should have written: For the Linux kernel the number of warnings dropped by 20% (going from level 3 to 1) and all of them turned out to be false positives. And yes, I have used "sort -u". I'm not sure if I would call 20% insignificant. -- Markus
Re: Change default level for -Wimplicit-fallthrough
On Thu, Nov 03, 2016 at 02:48:33PM +0100, Bernd Schmidt wrote: > > On 11/03/2016 02:47 PM, Jakub Jelinek wrote: > >On Thu, Nov 03, 2016 at 02:35:57PM +0100, Markus Trippelsdorf wrote: > >>I don't have gathered detailed statistics. But for example a simple > >>/* drop through */ in a package header file will of course cause many > >>bogus warnings during the build on level 2. > >>For the Linux kernel false positives decrease ~20% when switching from > >>level 3 to 1. > > > >One would have to count only warnings with unique locus (i.e. sort -u them > >after grepping them from logs). > >But even with 20%, if one spends the energy to analyze the 80%, where > >one actually has to analyze the code, just mechanically changing a couple of > >common comment kinds into more standardized one isn't going to be > >significant. > > But it's a completely pointless exercise which we shouldn't impose on users > unasked. It isn't pointless. Users can have completely unrelated comments in between case labels, to describe what the case handles etc. and then miss a warning when the fallthrough is not intentional. Jakub
Re: Change default level for -Wimplicit-fallthrough
On 11/03/2016 02:47 PM, Jakub Jelinek wrote: On Thu, Nov 03, 2016 at 02:35:57PM +0100, Markus Trippelsdorf wrote: I don't have gathered detailed statistics. But for example a simple /* drop through */ in a package header file will of course cause many bogus warnings during the build on level 2. For the Linux kernel false positives decrease ~20% when switching from level 3 to 1. One would have to count only warnings with unique locus (i.e. sort -u them after grepping them from logs). But even with 20%, if one spends the energy to analyze the 80%, where one actually has to analyze the code, just mechanically changing a couple of common comment kinds into more standardized one isn't going to be significant. But it's a completely pointless exercise which we shouldn't impose on users unasked. Bernd
Re: Change default level for -Wimplicit-fallthrough
On Thu, Nov 03, 2016 at 02:35:57PM +0100, Markus Trippelsdorf wrote: > I don't have gathered detailed statistics. But for example a simple > /* drop through */ in a package header file will of course cause many > bogus warnings during the build on level 2. > For the Linux kernel false positives decrease ~20% when switching from > level 3 to 1. One would have to count only warnings with unique locus (i.e. sort -u them after grepping them from logs). But even with 20%, if one spends the energy to analyze the 80%, where one actually has to analyze the code, just mechanically changing a couple of common comment kinds into more standardized one isn't going to be significant. Jakub
Re: Change default level for -Wimplicit-fallthrough
On 2016.11.03 at 14:24 +0100, Jakub Jelinek wrote: > On Thu, Nov 03, 2016 at 01:55:33PM +0100, Markus Trippelsdorf wrote: > > On 2016.11.03 at 13:32 +0100, Jakub Jelinek wrote: > > > On Thu, Nov 03, 2016 at 01:22:11PM +0100, Bernd Schmidt wrote: > > > > On 11/03/2016 12:58 PM, Jakub Jelinek wrote: > > > > >On Thu, Nov 03, 2016 at 12:51:15PM +0100, Bernd Schmidt wrote: > > > > >>I'm concerned about the number of false positives for this warning, > > > > >>and > > > > >>judging by previous discussions, I'm not alone in this. This patch > > > > >>limits it > > > > >>to level 1 (any comment before the case label disables the warning) > > > > >>for > > > > >>cases where the user specified no explicit level. It'll still generate > > > > >>enough noise that people will be aware of it and can choose whether > > > > >>to use a > > > > >>higher level or not. > > > > >> > > > > >>Bootstrapped and tested on x86_64-linux. Ok? > > > > > > > > > >I disagree, I'm ok with changing it to 2, but 1 is too much. > > > > > > > > Well, we have data from our own sources where we had to "fix" lots of > > > > perfectly good code, and also from the Linux kernel. From an earlier > > > > discussion: > > > > > > That data wasn't really convincing on this. All it proved is that most of > > > the cases are (likely) deliberate fall-throughs without any comment > > > whatsoever, the rest is in the noise. As one needs to deal with those > > > where comments are missing altogether, dealing with the noise is > > > acceptable. > > > > Without Bernd's patch to set the default to 1 you will drown in false > > positives once you start using gcc-7 to build a whole distro. On my > > Gentoo test box anything but level 1 is simply unacceptable, because you > > will miss important other warnings in the -Wimplicit-fallthrough noise > > otherwise. > > That is really strange. First of all, most of packages aren't compiled with > -Wextra/-W. And in those that are, are you sure that that no comment at all > for the implicit fallthroughs isn't significantly more common than the > set of comments that are accepted by -Wimplicit-fallthrough=1 and not > accepted by -Wimplicit-fallthrough=2? I don't have gathered detailed statistics. But for example a simple /* drop through */ in a package header file will of course cause many bogus warnings during the build on level 2. For the Linux kernel false positives decrease ~20% when switching from level 3 to 1. -- Markus
Re: Change default level for -Wimplicit-fallthrough
On Thu, Nov 03, 2016 at 01:55:33PM +0100, Markus Trippelsdorf wrote: > On 2016.11.03 at 13:32 +0100, Jakub Jelinek wrote: > > On Thu, Nov 03, 2016 at 01:22:11PM +0100, Bernd Schmidt wrote: > > > On 11/03/2016 12:58 PM, Jakub Jelinek wrote: > > > >On Thu, Nov 03, 2016 at 12:51:15PM +0100, Bernd Schmidt wrote: > > > >>I'm concerned about the number of false positives for this warning, and > > > >>judging by previous discussions, I'm not alone in this. This patch > > > >>limits it > > > >>to level 1 (any comment before the case label disables the warning) for > > > >>cases where the user specified no explicit level. It'll still generate > > > >>enough noise that people will be aware of it and can choose whether to > > > >>use a > > > >>higher level or not. > > > >> > > > >>Bootstrapped and tested on x86_64-linux. Ok? > > > > > > > >I disagree, I'm ok with changing it to 2, but 1 is too much. > > > > > > Well, we have data from our own sources where we had to "fix" lots of > > > perfectly good code, and also from the Linux kernel. From an earlier > > > discussion: > > > > That data wasn't really convincing on this. All it proved is that most of > > the cases are (likely) deliberate fall-throughs without any comment > > whatsoever, the rest is in the noise. As one needs to deal with those > > where comments are missing altogether, dealing with the noise is acceptable. > > Without Bernd's patch to set the default to 1 you will drown in false > positives once you start using gcc-7 to build a whole distro. On my > Gentoo test box anything but level 1 is simply unacceptable, because you > will miss important other warnings in the -Wimplicit-fallthrough noise > otherwise. That is really strange. First of all, most of packages aren't compiled with -Wextra/-W. And in those that are, are you sure that that no comment at all for the implicit fallthroughs isn't significantly more common than the set of comments that are accepted by -Wimplicit-fallthrough=1 and not accepted by -Wimplicit-fallthrough=2? Jakub
Re: Change default level for -Wimplicit-fallthrough
On 2016.11.03 at 13:32 +0100, Jakub Jelinek wrote: > On Thu, Nov 03, 2016 at 01:22:11PM +0100, Bernd Schmidt wrote: > > On 11/03/2016 12:58 PM, Jakub Jelinek wrote: > > >On Thu, Nov 03, 2016 at 12:51:15PM +0100, Bernd Schmidt wrote: > > >>I'm concerned about the number of false positives for this warning, and > > >>judging by previous discussions, I'm not alone in this. This patch limits > > >>it > > >>to level 1 (any comment before the case label disables the warning) for > > >>cases where the user specified no explicit level. It'll still generate > > >>enough noise that people will be aware of it and can choose whether to > > >>use a > > >>higher level or not. > > >> > > >>Bootstrapped and tested on x86_64-linux. Ok? > > > > > >I disagree, I'm ok with changing it to 2, but 1 is too much. > > > > Well, we have data from our own sources where we had to "fix" lots of > > perfectly good code, and also from the Linux kernel. From an earlier > > discussion: > > That data wasn't really convincing on this. All it proved is that most of > the cases are (likely) deliberate fall-throughs without any comment > whatsoever, the rest is in the noise. As one needs to deal with those > where comments are missing altogether, dealing with the noise is acceptable. Without Bernd's patch to set the default to 1 you will drown in false positives once you start using gcc-7 to build a whole distro. On my Gentoo test box anything but level 1 is simply unacceptable, because you will miss important other warnings in the -Wimplicit-fallthrough noise otherwise. -- Markus
Re: Change default level for -Wimplicit-fallthrough
On Thu, Nov 03, 2016 at 01:22:11PM +0100, Bernd Schmidt wrote: > On 11/03/2016 12:58 PM, Jakub Jelinek wrote: > >On Thu, Nov 03, 2016 at 12:51:15PM +0100, Bernd Schmidt wrote: > >>I'm concerned about the number of false positives for this warning, and > >>judging by previous discussions, I'm not alone in this. This patch limits it > >>to level 1 (any comment before the case label disables the warning) for > >>cases where the user specified no explicit level. It'll still generate > >>enough noise that people will be aware of it and can choose whether to use a > >>higher level or not. > >> > >>Bootstrapped and tested on x86_64-linux. Ok? > > > >I disagree, I'm ok with changing it to 2, but 1 is too much. > > Well, we have data from our own sources where we had to "fix" lots of > perfectly good code, and also from the Linux kernel. From an earlier > discussion: That data wasn't really convincing on this. All it proved is that most of the cases are (likely) deliberate fall-throughs without any comment whatsoever, the rest is in the noise. As one needs to deal with those where comments are missing altogether, dealing with the noise is acceptable. > Markus Trippelsdorf wrote: > >>>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. > [...] > >Actually looking more closely it appears that all of the 136 additional > >warnings for level 2 are bogus, too. > > Also, levels above 1 enforce English as a language, which isn't something we > should be doing, even if we could detect fallthrough comments reliably (and > we can't). If you use non-english comment, then you need to add lint-like comments for this, or attributes. IMHO it is really not very high cost (compared to all those without any comment at all) to adjust code, what takes time is to analyze if something is intentional or invalid fallthrough. The clearer the comments are on that, the better. Jakub
Re: Change default level for -Wimplicit-fallthrough
On 11/03/2016 12:58 PM, Jakub Jelinek wrote: On Thu, Nov 03, 2016 at 12:51:15PM +0100, Bernd Schmidt wrote: I'm concerned about the number of false positives for this warning, and judging by previous discussions, I'm not alone in this. This patch limits it to level 1 (any comment before the case label disables the warning) for cases where the user specified no explicit level. It'll still generate enough noise that people will be aware of it and can choose whether to use a higher level or not. Bootstrapped and tested on x86_64-linux. Ok? I disagree, I'm ok with changing it to 2, but 1 is too much. Well, we have data from our own sources where we had to "fix" lots of perfectly good code, and also from the Linux kernel. From an earlier discussion: Markus Trippelsdorf wrote: 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. [...] Actually looking more closely it appears that all of the 136 additional warnings for level 2 are bogus, too. Also, levels above 1 enforce English as a language, which isn't something we should be doing, even if we could detect fallthrough comments reliably (and we can't). Bernd
Re: Change default level for -Wimplicit-fallthrough
On Thu, Nov 03, 2016 at 12:58:55PM +0100, Jakub Jelinek wrote: > On Thu, Nov 03, 2016 at 12:51:15PM +0100, Bernd Schmidt wrote: > > I'm concerned about the number of false positives for this warning, and > > judging by previous discussions, I'm not alone in this. This patch limits it > > to level 1 (any comment before the case label disables the warning) for > > cases where the user specified no explicit level. It'll still generate > > enough noise that people will be aware of it and can choose whether to use a > > higher level or not. > > > > Bootstrapped and tested on x86_64-linux. Ok? > > I disagree, I'm ok with changing it to 2, but 1 is too much. Same here. I'd support the level 2. Marek
Re: Change default level for -Wimplicit-fallthrough
On Thu, Nov 03, 2016 at 12:51:15PM +0100, Bernd Schmidt wrote: > I'm concerned about the number of false positives for this warning, and > judging by previous discussions, I'm not alone in this. This patch limits it > to level 1 (any comment before the case label disables the warning) for > cases where the user specified no explicit level. It'll still generate > enough noise that people will be aware of it and can choose whether to use a > higher level or not. > > Bootstrapped and tested on x86_64-linux. Ok? I disagree, I'm ok with changing it to 2, but 1 is too much. Jakub
