In PR70405 I've run into something we should IMNSHO definitely warn about: void bar (int, int); void foo (int x, int y, int z) { int i; if (x) for (i = 0; i < 64; i++) if (y) bar (1, i); else for (i = 0; i < 64; i++) if (z) bar (2, i); } but -W -Wall is quiet on this. Here the else is indented as the code was meant to work, but is actually parsed as if (x) for (i = 0; i < 64; i++) if (u) bar (1, i); else for (i = 0; i < 64; i++) if (z) bar (2, i);
Note clang warns here: pr70405-3.c:11:3: warning: add explicit braces to avoid dangling else [-Wdangling-else] else ^ and it warns regardless of the indentation.
This may be tricky to support in -Wmisleading-indentation without introducing false-positives because we only have the last 3 tokens to work with and don't know about the location or existence of the topmost if(). It should be easy to implement a separate -Wdangling-else warning though that keeps track of the number of unbraced if()s and warns when encountering an else with >= 2 unbraced if()s.
FWIW, I also think we should add -Wdangling-else rather than wiring this into -Wmisleading-indentation. Confirmed in any case.
Here's a prototype that seems to do the job. Whenever we see an unbraced if, increment the counter. Whenever we see a compound statement, reset the counter. Whenever we see an else, warn if counter > 1 and then decrement the counter. diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 2f80856..3ad7196 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -10738,6 +10738,8 @@ cp_parser_expression_statement (cp_parser* parser, tree in_statement_expr) Returns a tree representing the statement. */ +static int num_unbraced_if_statements; + static tree cp_parser_compound_statement (cp_parser *parser, tree in_statement_expr, int bcs_flags, bool function_body) @@ -10747,6 +10749,7 @@ cp_parser_compound_statement (cp_parser *parser, tree in_statement_expr, /* Consume the `{'. */ if (!cp_parser_require (parser, CPP_OPEN_BRACE, RT_OPEN_BRACE)) return error_mark_node; + num_unbraced_if_statements = 0; if (DECL_DECLARED_CONSTEXPR_P (current_function_decl) && !function_body && cxx_dialect < cxx14) pedwarn (input_location, OPT_Wpedantic, @@ -10874,6 +10877,7 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, bool nested_if; unsigned char in_statement; + num_unbraced_if_statements++; /* Add the condition. */ finish_if_stmt_cond (condition, statement); @@ -10898,6 +10902,9 @@ cp_parser_selection_statement (cp_parser* parser, bool *if_p, = get_token_indent_info (cp_lexer_peek_token (parser->lexer)); /* Consume the `else' keyword. */ cp_lexer_consume_token (parser->lexer); + if (num_unbraced_if_statements > 1) + warning_at (input_location, 0, "dangling else"); + num_unbraced_if_statements--; if (warn_duplicated_cond) { if (cp_lexer_next_token_is_keyword (parser->lexer, @@ -11953,6 +11960,7 @@ cp_parser_already_scoped_statement (cp_parser* parser, } else { + num_unbraced_if_statements = 0; /* Avoid calling cp_parser_compound_statement, so that we don't create a new scope. Do everything else by hand. */ cp_parser_require (parser, CPP_OPEN_BRACE, RT_OPEN_BRACE);
Thanks. I could take care of the C FE side ;). But let's strike this in the next stage1.
Turns out the prototype is pretty broken :P
BTW according to a working implementation of this -Wdangling-else warning (not the above broken one I prematurely posted), the only suspicious use of dangling else (i.e. a use in which the indentation is off*) in gcc's c++ source files is /home/patrick/code/gcc/gcc/ssa-iterators.h:454:3: note: dangling else *: by indentation being off I mean that the column of the dangling else is not the same as the column of the innermost if
This should be part of -Wparentheses, where we already have dangling else warnings (called "ambiguous else"). Earlier versions of gcc warned for this case.
I suspect what broke it was git revision 0375a27521885.
I'd consider it a bug that the existing -Wparentheses warning for ambiguous "else" doesn't warn in this case. As with the existing warning, that warning is only appropriate when there is in fact ambiguity (thus, not if there were two more "else" clauses in this example so that every "if" had one and there was no ambiguity), though the indentation could still be misleading without such ambiguity. I think the existing warning, controlled by the existing option (or any new option that's a subset of -Wparentheses and enabled by it), should apply to all cases where the syntax for statements permits multiple parses differing in which "if" an "else" is associated with, and the rule associating it with the nearest has to be applied to select between those alternatives. (Obviously global state as in the prototype is not a correct part of any sensible implementation of such a warning, as you need to keep track of arbitrarily many nested states for "if" statements currently being parsed.)
Should non-standard constructs be considered in this PR? I noticed that we also don't warn on if (a) #pragma GCC ivdep while (1) if (b) bar (); else baz (); and if (a) _Cilk_for (int i = 0; i < 10; i++) if (b) bar (); else baz (); and probably other such constructs.
They should be. if (x) #pragma omp for for (...) if (y) ... else ... and #pragma omp simd and #pragma omp taskloop too. For C++, perhaps we could just pass around if_p argument to a few more parsing functions.
Author: ppalka Date: Wed Apr 6 23:07:21 2016 New Revision: 234801 URL: https://gcc.gnu.org/viewcvs?rev=234801&root=gcc&view=rev Log: Fix new -Wparentheses warnings encountered during bootstrap gcc/ChangeLog: PR c/70436 * gimplify.c (gimplify_omp_ordered): Add explicit braces to resolve a future -Wparentheses warning. * omp-low.c (scan_sharing_clauses): Likewise. * tree-parloops.c (eliminate_local_variables): Likewise. gcc/cp/ChangeLog: PR c/70436 * cp-tree.h (FOR_EACH_CLONE): Restructure macro to avoid potentially generating a future -Wparentheses warning in its callers. gcc/fortran/ChangeLog: PR c/70436 * openmp.c (gfc_find_omp_udr): Add explicit braces to resolve a future -Wparentheses warning. gcc/testsuite/ChangeLog: PR c/70436 * g++.dg/plugin/pragma_plugin.c (handle_pragma_sayhello): Add explicit braces to resolve a future -Wparentheses warning. Modified: trunk/gcc/ChangeLog trunk/gcc/cp/ChangeLog trunk/gcc/cp/cp-tree.h trunk/gcc/fortran/ChangeLog trunk/gcc/fortran/openmp.c trunk/gcc/gimplify.c trunk/gcc/omp-low.c trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/g++.dg/plugin/pragma_plugin.c trunk/gcc/tree-parloops.c
Author: ppalka Date: Wed Apr 6 23:10:14 2016 New Revision: 234802 URL: https://gcc.gnu.org/viewcvs?rev=234802&root=gcc&view=rev Log: Fix C++ side of PR c/70436 (missing -Wparentheses warnings) gcc/cp/ChangeLog: PR c/70436 * parser.c (cp_parser_iteration_statement): New parameter IF_P. Pass it through to cp_parser_already_scoped_statement. (cp_parser_already_scoped_statement): New parameter IF_P. Pass it through to cp_parser_statement. (cp_parser_statement): Pass IF_P through to cp_parser_iteration_statement. (cp_parser_pragma): Adjust call to cp_parser_iteration_statement. gcc/testsuite/ChangeLog: PR c/70436 * g++.dg/warn/Wparentheses-29.C: New test. Added: trunk/gcc/testsuite/g++.dg/warn/Wparentheses-29.C Modified: trunk/gcc/ChangeLog trunk/gcc/cp/parser.c trunk/gcc/testsuite/ChangeLog
It's now up to me to handle the C FE side.
Author: mpolacek Date: Wed Apr 13 16:00:52 2016 New Revision: 234949 URL: https://gcc.gnu.org/viewcvs?rev=234949&root=gcc&view=rev Log: PR c/70436 * c-parser.c (c_parser_statement_after_labels): Add IF_P argument and adjust callers. (c_parser_statement): Likewise. (c_parser_c99_block_statement): Likewise. (c_parser_while_statement): Likewise. (c_parser_for_statement): Likewise. (c_parser_if_body): Don't set IF_P here. (c_parser_if_statement): Add IF_P argument. Set IF_P here. Warn about dangling else here. * c-tree.h (c_finish_if_stmt): Adjust declaration. * c-typeck.c (c_finish_if_stmt): Remove NESTED_IF parameter. Don't warn about dangling else here. * testsuite/gcc.dg/Wparentheses-12.c: New test. * testsuite/gcc.dg/Wparentheses-13.c: New test. Added: trunk/gcc/testsuite/gcc.dg/Wparentheses-12.c trunk/gcc/testsuite/gcc.dg/Wparentheses-13.c Modified: trunk/gcc/c/ChangeLog trunk/gcc/c/c-parser.c trunk/gcc/c/c-tree.h trunk/gcc/c/c-typeck.c trunk/gcc/testsuite/ChangeLog
Should be fixed now.
Created attachment 38279 [details] gcc6-pr70436-omp.patch Untested fix for OpenMP/OpenACC/Cilk+/#pragma GCC ivdep, both C and C++.
Author: jakub Date: Fri Apr 15 12:24:18 2016 New Revision: 235020 URL: https://gcc.gnu.org/viewcvs?rev=235020&root=gcc&view=rev Log: PR c/70436 c/ * c-parser.c (c_parser_pragma): Add IF_P argument, pass it down where needed. (c_parser_external_declaration, c_parser_struct_or_union_specifier, c_parser_parameter_declaration, c_parser_compound_statement_nostart, c_parser_objc_class_instance_variables, c_parser_objc_methodprotolist): Adjust c_parser_pragma callers. (c_parser_statement_after_labels): Likewise. Adjust c_parser_cilk_for caller. (c_parser_omp_structured_block): Add IF_P argument, pass it down to c_parser_statement. (c_parser_oacc_data, c_parser_oacc_host_data, c_parser_oacc_loop, c_parser_oacc_kernels_parallel, c_parser_omp_critical, c_parser_omp_simd, c_parser_omp_for, c_parser_omp_master, c_parser_omp_ordered, c_parser_omp_parallel, c_parser_omp_single, c_parser_omp_task, c_parser_omp_taskgroup, c_parser_omp_distribute, c_parser_omp_teams, c_parser_omp_target_data, c_parser_omp_target, c_parser_omp_taskloop, c_parser_omp_construct, c_parser_cilk_grainsize, c_parser_cilk_simd, c_parser_cilk_for): Add IF_P argument, pass it down where needed. (c_parser_omp_for_loop): Likewise. Clear IF_P if nbraces. (c_parser_omp_sections_scope): Adjust c_parser_omp_structured_block calls. cp/ * parser.c (cp_parser_pragma): Add IF_P argument, pass it down where needed. (cp_parser_declaration_seq_opt, cp_parser_member_specification_opt, cp_parser_objc_interstitial_code, cp_parser_omp_declare_simd, cp_parser_oacc_routine): Adjust cp_parser_pragma callers. (cp_parser_statement): Likewise. Adjust cp_parser_cilk_for caller. (cp_parser_omp_structured_block): Add IF_P argument, pass it down to cp_parser_statement. (cp_parser_oacc_data, cp_parser_oacc_host_data, cp_parser_oacc_loop, cp_parser_oacc_kernels_parallel, cp_parser_omp_critical, cp_parser_omp_simd, cp_parser_omp_for, cp_parser_omp_master, cp_parser_omp_ordered, cp_parser_omp_parallel, cp_parser_omp_single, cp_parser_omp_task, cp_parser_omp_taskgroup, cp_parser_omp_distribute, cp_parser_omp_teams, cp_parser_omp_target_data, cp_parser_omp_target, cp_parser_omp_taskloop, cp_parser_omp_construct, cp_parser_cilk_grainsize, cp_parser_cilk_simd, cp_parser_cilk_for): Add IF_P argument, pass it down where needed. (cp_parser_omp_for_loop): Likewise. Clear IF_P if nbraces. (cp_parser_omp_sections_scope): Adjust cp_parser_omp_structured_block calls. testsuite/ * c-c++-common/Wparentheses-1.c: New test. * c-c++-common/gomp/Wparentheses-1.c: New test. * c-c++-common/gomp/Wparentheses-2.c: New test. * c-c++-common/gomp/Wparentheses-3.c: New test. * c-c++-common/gomp/Wparentheses-4.c: New test. * c-c++-common/cilk-plus/PS/Wparentheses-1.c: New test. * c-c++-common/cilk-plus/CK/Wparentheses-1.c: New test. * c-c++-common/goacc/Wparentheses-1.c: New test. Added: trunk/gcc/testsuite/c-c++-common/Wparentheses-1.c trunk/gcc/testsuite/c-c++-common/cilk-plus/CK/Wparentheses-1.c trunk/gcc/testsuite/c-c++-common/cilk-plus/PS/Wparentheses-1.c trunk/gcc/testsuite/c-c++-common/goacc/Wparentheses-1.c trunk/gcc/testsuite/c-c++-common/gomp/Wparentheses-1.c trunk/gcc/testsuite/c-c++-common/gomp/Wparentheses-2.c trunk/gcc/testsuite/c-c++-common/gomp/Wparentheses-3.c trunk/gcc/testsuite/c-c++-common/gomp/Wparentheses-4.c Modified: trunk/gcc/c/ChangeLog trunk/gcc/c/c-parser.c trunk/gcc/cp/ChangeLog trunk/gcc/cp/parser.c trunk/gcc/testsuite/ChangeLog