| Message ID | 20160413141444.GT28445@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 13, 2016 at 04:14:44PM +0200, Marek Polacek wrote: > This patch is meant to be applied on top of the "Wparentheses overhaul" patch. > > I really think that warning about the dangling else problem isn't appropriate > as a part of the -Wparentheses warning, which I think should only deal with > stuff like precedence of operators, i.e. things where ()'s are missing and not > {}'s. > > This new warning is, however, a subset of -Wparentheses. > > Bootstrapped/regtested on x86_64-linux, ok for trunk or should I stash it > for the next stage1? > > 2016-04-13 Marek Polacek <polacek@redhat.com> > > * c.opt (Wdangling-else): New option. > > * c-parser.c (c_parser_if_statement): Replace OPT_Wparentheses with > OPT_Wdangling_else. > > * parser.c (cp_parser_selection_statement): Replace OPT_Wparentheses > with OPT_Wdangling_else. > > * doc/invoke.texi: Document -Wdangling-else. > > * c-c++-common/Wdangling-else-1.c: New test. > * c-c++-common/Wdangling-else-2.c: New test. > * c-c++-common/Wdangling-else-3.c: New test. LGTM, though I think it would be useful to include in invoke.texi also a small example with for to make it clear the option also handles that. Jakub
On 04/13/2016 04:14 PM, Marek Polacek wrote: > This patch is meant to be applied on top of the "Wparentheses overhaul" patch. > > I really think that warning about the dangling else problem isn't appropriate > as a part of the -Wparentheses warning, which I think should only deal with > stuff like precedence of operators, i.e. things where ()'s are missing and not > {}'s. > > This new warning is, however, a subset of -Wparentheses. > > Bootstrapped/regtested on x86_64-linux, ok for trunk or should I stash it > for the next stage1? I think it's not appropriate for now. I'm ambivalent about the concept; my (vague) recollection is that putting it under -Wparentheses was Kenner's idea, and it's been there so long that I'm not sure there's really a point to changing this. In a sense it is a very similar problem as operator precedence. Bernd
On Wed, Apr 13, 2016 at 05:13:28PM +0200, Bernd Schmidt wrote: > On 04/13/2016 04:14 PM, Marek Polacek wrote: > >This patch is meant to be applied on top of the "Wparentheses overhaul" patch. > > > >I really think that warning about the dangling else problem isn't appropriate > >as a part of the -Wparentheses warning, which I think should only deal with > >stuff like precedence of operators, i.e. things where ()'s are missing and not > >{}'s. > > > >This new warning is, however, a subset of -Wparentheses. > > > >Bootstrapped/regtested on x86_64-linux, ok for trunk or should I stash it > >for the next stage1? > > I think it's not appropriate for now. I'm ambivalent about the concept; my > (vague) recollection is that putting it under -Wparentheses was Kenner's > idea, and it's been there so long that I'm not sure there's really a point > to changing this. In a sense it is a very similar problem as operator > precedence. Well, even with the change it is still included with -Wparentheses, just it is a suboption with more specific name that can be enabled/disabled independently from -Wparentheses if needed. Though, of course, it can wait for GCC 7. Jakub
On Wed, Apr 13, 2016 at 05:16:12PM +0200, Jakub Jelinek wrote: > On Wed, Apr 13, 2016 at 05:13:28PM +0200, Bernd Schmidt wrote: > > On 04/13/2016 04:14 PM, Marek Polacek wrote: > > >This patch is meant to be applied on top of the "Wparentheses overhaul" patch. > > > > > >I really think that warning about the dangling else problem isn't appropriate > > >as a part of the -Wparentheses warning, which I think should only deal with > > >stuff like precedence of operators, i.e. things where ()'s are missing and not > > >{}'s. > > > > > >This new warning is, however, a subset of -Wparentheses. > > > > > >Bootstrapped/regtested on x86_64-linux, ok for trunk or should I stash it > > >for the next stage1? > > > > I think it's not appropriate for now. I'm ambivalent about the concept; my > > (vague) recollection is that putting it under -Wparentheses was Kenner's > > idea, and it's been there so long that I'm not sure there's really a point > > to changing this. In a sense it is a very similar problem as operator > > precedence. > > Well, even with the change it is still included with -Wparentheses, just > it is a suboption with more specific name that can be enabled/disabled > independently from -Wparentheses if needed. > Though, of course, it can wait for GCC 7. So how do y'all feel about this patch now that we're in stage1? Marek
On Tue, Apr 26, 2016 at 02:32:01PM +0200, Marek Polacek wrote: > On Wed, Apr 13, 2016 at 05:16:12PM +0200, Jakub Jelinek wrote: > > On Wed, Apr 13, 2016 at 05:13:28PM +0200, Bernd Schmidt wrote: > > > On 04/13/2016 04:14 PM, Marek Polacek wrote: > > > >This patch is meant to be applied on top of the "Wparentheses overhaul" patch. > > > > > > > >I really think that warning about the dangling else problem isn't appropriate > > > >as a part of the -Wparentheses warning, which I think should only deal with > > > >stuff like precedence of operators, i.e. things where ()'s are missing and not > > > >{}'s. > > > > > > > >This new warning is, however, a subset of -Wparentheses. > > > > > > > >Bootstrapped/regtested on x86_64-linux, ok for trunk or should I stash it > > > >for the next stage1? > > > > > > I think it's not appropriate for now. I'm ambivalent about the concept; my > > > (vague) recollection is that putting it under -Wparentheses was Kenner's > > > idea, and it's been there so long that I'm not sure there's really a point > > > to changing this. In a sense it is a very similar problem as operator > > > precedence. > > > > Well, even with the change it is still included with -Wparentheses, just > > it is a suboption with more specific name that can be enabled/disabled > > independently from -Wparentheses if needed. > > Though, of course, it can wait for GCC 7. > > So how do y'all feel about this patch now that we're in stage1? I support that change, and -Wparentheses will still enable this, it just gives more fine-grained control and be in line with what clang does. Bernd, how much are you against this change? Jakub
On 04/26/2016 02:39 PM, Jakub Jelinek wrote: > I support that change, and -Wparentheses will still enable this, it just > gives more fine-grained control and be in line with what clang does. > > Bernd, how much are you against this change? Don't really care that much, I just don't quite see the point. Don't let me stop you though. Bernd
On Tue, Apr 26, 2016 at 03:03:25PM +0200, Bernd Schmidt wrote: > On 04/26/2016 02:39 PM, Jakub Jelinek wrote: > > I support that change, and -Wparentheses will still enable this, it just > > gives more fine-grained control and be in line with what clang does. > > > > Bernd, how much are you against this change? > > Don't really care that much, I just don't quite see the point. Don't let me > stop you though. So Joseph, what do you think about this patch? :) Marek
On Wed, 4 May 2016, Marek Polacek wrote: > On Tue, Apr 26, 2016 at 03:03:25PM +0200, Bernd Schmidt wrote: > > On 04/26/2016 02:39 PM, Jakub Jelinek wrote: > > > I support that change, and -Wparentheses will still enable this, it just > > > gives more fine-grained control and be in line with what clang does. > > > > > > Bernd, how much are you against this change? > > > > Don't really care that much, I just don't quite see the point. Don't let me > > stop you though. > > So Joseph, what do you think about this patch? :) I support adding the option for more fine-grained control of these warnings.
On Wed, May 04, 2016 at 03:39:19PM +0000, Joseph Myers wrote: > On Wed, 4 May 2016, Marek Polacek wrote: > > > On Tue, Apr 26, 2016 at 03:03:25PM +0200, Bernd Schmidt wrote: > > > On 04/26/2016 02:39 PM, Jakub Jelinek wrote: > > > > I support that change, and -Wparentheses will still enable this, it just > > > > gives more fine-grained control and be in line with what clang does. > > > > > > > > Bernd, how much are you against this change? > > > > > > Don't really care that much, I just don't quite see the point. Don't let me > > > stop you though. > > > > So Joseph, what do you think about this patch? :) > > I support adding the option for more fine-grained control of these > warnings. Thanks. I'll commit the patch then. Marek
diff --git gcc/c-family/c.opt gcc/c-family/c.opt index 4f86876..5e7bcb8 100644 --- gcc/c-family/c.opt +++ gcc/c-family/c.opt @@ -370,6 +370,10 @@ Wctor-dtor-privacy C++ ObjC++ Var(warn_ctor_dtor_privacy) Warning Warn when all constructors and destructors are private. +Wdangling-else +C ObjC C++ ObjC++ Var(warn_dangling_else) Warning LangEnabledBy(C ObjC C++ ObjC++,Wparentheses) +Warn about dangling else. + Wdate-time C ObjC C++ ObjC++ CPP(warn_date_time) CppReason(CPP_W_DATE_TIME) Var(cpp_warn_date_time) Init(0) Warning Warn about __TIME__, __DATE__ and __TIMESTAMP__ usage. diff --git gcc/c/c-parser.c gcc/c/c-parser.c index cafcb99..1f95446 100644 --- gcc/c/c-parser.c +++ gcc/c/c-parser.c @@ -5486,7 +5486,7 @@ c_parser_if_statement (c_parser *parser, bool *if_p, vec<tree> *chain) /* Diagnose an ambiguous else if if-then-else is nested inside if-then. */ if (nested_if) - warning_at (loc, OPT_Wparentheses, + warning_at (loc, OPT_Wdangling_else, "suggest explicit braces to avoid ambiguous %<else%>"); if (warn_duplicated_cond) diff --git gcc/cp/parser.c gcc/cp/parser.c index 00e211e..968706f 100644 --- gcc/cp/parser.c +++ gcc/cp/parser.c @@ -10951,7 +10951,7 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, statement which does have an else clause. We warn about the potential ambiguity. */ if (nested_if) - warning_at (EXPR_LOCATION (statement), OPT_Wparentheses, + warning_at (EXPR_LOCATION (statement), OPT_Wdangling_else, "suggest explicit braces to avoid ambiguous" " %<else%>"); if (warn_duplicated_cond) diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi index e9763d4..6253d78 100644 --- gcc/doc/invoke.texi +++ gcc/doc/invoke.texi @@ -257,7 +257,8 @@ Objective-C and Objective-C++ Dialects}. -Wc90-c99-compat -Wc99-c11-compat @gol -Wc++-compat -Wc++11-compat -Wc++14-compat -Wcast-align -Wcast-qual @gol -Wchar-subscripts -Wclobbered -Wcomment -Wconditionally-supported @gol --Wconversion -Wcoverage-mismatch -Wno-cpp -Wdate-time -Wdelete-incomplete @gol +-Wconversion -Wcoverage-mismatch -Wno-cpp -Wdangling-else -Wdate-time @gol +-Wdelete-incomplete @gol -Wno-deprecated -Wno-deprecated-declarations -Wno-designated-init @gol -Wdisabled-optimization @gol -Wno-discarded-qualifiers -Wno-discarded-array-qualifiers @gol @@ -3974,46 +3975,6 @@ Also warn if a comparison like @code{x<=y<=z} appears; this is equivalent to @code{(x<=y ? 1 : 0) <= z}, which is a different interpretation from that of ordinary mathematical notation. -Also warn about constructions where there may be confusion to which -@code{if} statement an @code{else} branch belongs. Here is an example of -such a case: - -@smallexample -@group -@{ - if (a) - if (b) - foo (); - else - bar (); -@} -@end group -@end smallexample - -In C/C++, every @code{else} branch belongs to the innermost possible -@code{if} statement, which in this example is @code{if (b)}. This is -often not what the programmer expected, as illustrated in the above -example by indentation the programmer chose. When there is the -potential for this confusion, GCC issues a warning when this flag -is specified. To eliminate the warning, add explicit braces around -the innermost @code{if} statement so there is no way the @code{else} -can belong to the enclosing @code{if}. The resulting code -looks like this: - -@smallexample -@group -@{ - if (a) - @{ - if (b) - foo (); - else - bar (); - @} -@} -@end group -@end smallexample - Also warn for dangerous uses of the GNU extension to @code{?:} with omitted middle operand. When the condition in the @code{?}: operator is a boolean expression, the omitted value is @@ -5146,6 +5107,51 @@ compiler doesn't give this warning for types defined in the main .C file, as those are unlikely to have multiple definitions. @option{-Wsubobject-linkage} is enabled by default. +@item -Wdangling-else +@opindex Wdangling-else +@opindex Wno-dangling-else +Warn about constructions where there may be confusion to which +@code{if} statement an @code{else} branch belongs. Here is an example of +such a case: + +@smallexample +@group +@{ + if (a) + if (b) + foo (); + else + bar (); +@} +@end group +@end smallexample + +In C/C++, every @code{else} branch belongs to the innermost possible +@code{if} statement, which in this example is @code{if (b)}. This is +often not what the programmer expected, as illustrated in the above +example by indentation the programmer chose. When there is the +potential for this confusion, GCC issues a warning when this flag +is specified. To eliminate the warning, add explicit braces around +the innermost @code{if} statement so there is no way the @code{else} +can belong to the enclosing @code{if}. The resulting code +looks like this: + +@smallexample +@group +@{ + if (a) + @{ + if (b) + foo (); + else + bar (); + @} +@} +@end group +@end smallexample + +This warning is enabled by @option{-Wparentheses}. + @item -Wdate-time @opindex Wdate-time @opindex Wno-date-time diff --git gcc/testsuite/c-c++-common/Wdangling-else-1.c gcc/testsuite/c-c++-common/Wdangling-else-1.c index e69de29..28a5a8f 100644 --- gcc/testsuite/c-c++-common/Wdangling-else-1.c +++ gcc/testsuite/c-c++-common/Wdangling-else-1.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-Wdangling-else" } */ + +void bar (int); +void +foo (int a, int b) +{ + if (a) /* { dg-warning "suggest explicit braces to avoid ambiguous" } */ + if (b) + bar (1); + else + bar (2); +} diff --git gcc/testsuite/c-c++-common/Wdangling-else-2.c gcc/testsuite/c-c++-common/Wdangling-else-2.c index e69de29..87ea1ab 100644 --- gcc/testsuite/c-c++-common/Wdangling-else-2.c +++ gcc/testsuite/c-c++-common/Wdangling-else-2.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-Wparentheses" } */ + +void bar (int); +void +foo (int a, int b) +{ + if (a) /* { dg-warning "suggest explicit braces to avoid ambiguous" } */ + if (b) + bar (1); + else + bar (2); +} diff --git gcc/testsuite/c-c++-common/Wdangling-else-3.c gcc/testsuite/c-c++-common/Wdangling-else-3.c index e69de29..0dae0d5 100644 --- gcc/testsuite/c-c++-common/Wdangling-else-3.c +++ gcc/testsuite/c-c++-common/Wdangling-else-3.c @@ -0,0 +1,13 @@ +/* { dg-do compile } */ +/* { dg-options "-Wparentheses -Wno-dangling-else" } */ + +void bar (int); +void +foo (int a, int b) +{ + if (a) /* { dg-bogus "suggest explicit braces to avoid ambiguous" } */ + if (b) + bar (1); + else + bar (2); +}
This patch is meant to be applied on top of the "Wparentheses overhaul" patch. I really think that warning about the dangling else problem isn't appropriate as a part of the -Wparentheses warning, which I think should only deal with stuff like precedence of operators, i.e. things where ()'s are missing and not {}'s. This new warning is, however, a subset of -Wparentheses. Bootstrapped/regtested on x86_64-linux, ok for trunk or should I stash it for the next stage1? 2016-04-13 Marek Polacek <polacek@redhat.com> * c.opt (Wdangling-else): New option. * c-parser.c (c_parser_if_statement): Replace OPT_Wparentheses with OPT_Wdangling_else. * parser.c (cp_parser_selection_statement): Replace OPT_Wparentheses with OPT_Wdangling_else. * doc/invoke.texi: Document -Wdangling-else. * c-c++-common/Wdangling-else-1.c: New test. * c-c++-common/Wdangling-else-2.c: New test. * c-c++-common/Wdangling-else-3.c: New test. Marek