Comment #1 of PR c/68187 identified another overzealous warning from -Wmisleading-indentation, with OpenSSL 1.0.1, on this poorly indented code:
115 if (locked) 116 i = CRYPTO_add(&e->struct_ref, -1, CRYPTO_LOCK_ENGINE); 117 else 118 i = --e->struct_ref; 119 engine_ref_debug(e, 0, -1) 120 if (i > 0) 121 return 1; eng_lib.c:120:9: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation] if (i > 0) ^~ eng_lib.c:117:5: note: ...this 'else' clause, but it is not else ^~~~ Line 120 is poorly indented, but the warning is arguably unjustified. Root cause is that "engine_ref_debug" is actually a debugging macro that was empty in the given configuration, so the code effectively was: 117 else // GUARD 118 i = --e->struct_ref; // BODY 119 120 if (i > 0) // NEXT hence the warning. But the code as seen by a human is clearly *not* misleading, and hence arguably we shouldn't warn for this case. The following patch fixes this by ruling that if there is non-whitespace in a line between the BODY and the NEXT statements, and that this non-whitespace is effectively an "unindent" or "outdent" (it's not clear to me which of these terms is better), then to not issue a warning. In doing so I eliminated one of the existing heuristics: we already had code to ignore preprocessor directives between BODY and NEXT for cases like this: if (flagA) // GUARD foo (0); // BODY #if SOME_CONDITION_THAT_DOES_NOT_HOLD if (flagB) #endif foo (1); // NEXT This is handled by the new heuristic, so the new heuristic simply replaces the old one. Sadly the replacement of two old functions with two new functions leads to a rather messy diff within c-indentation.c; I can split it up into a removal/addition pair of patches if that's easier to review. Successfully bootstrapped®rtested on x86_64-pc-linux-gnu (in combination with the previous patch). OK for trunk? Note: one of the new test cases adds a dg-warning/dg-message pair, and so would require updating if/when the wording change posted here: https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00068.html ("[PATCH] PR c/69993: improvements to wording of -Wmisleading-indentation") is applied. gcc/c-family/ChangeLog: PR c/68187 * c-indentation.c (get_visual_column): Move code to determine next tab stop to... (next_tab_stop): ...this new function. (line_contains_hash_if): Delete function. (detect_preprocessor_logic): Delete function. (get_first_nws_vis_column): New function. (detect_intervening_unindent): New function. (should_warn_for_misleading_indentation): Replace call to detect_preprocessor_logic with a call to detect_intervening_unindent. gcc/testsuite/ChangeLog: PR c/68187 * c-c++-common/Wmisleading-indentation.c (fn_42_a): New test function. (fn_42_b): Likewise. (fn_42_c): Likewise. --- gcc/c-family/c-indentation.c | 141 ++++++++++++--------- .../c-c++-common/Wmisleading-indentation.c | 72 +++++++++++ 2 files changed, 152 insertions(+), 61 deletions(-) diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c index c72192d..b84fbf4 100644 --- a/gcc/c-family/c-indentation.c +++ b/gcc/c-family/c-indentation.c @@ -26,6 +26,16 @@ along with GCC; see the file COPYING3. If not see extern cpp_options *cpp_opts; +/* Round up VIS_COLUMN to nearest tab stop. */ + +static unsigned int +next_tab_stop (unsigned int vis_column) +{ + const unsigned int tab_width = cpp_opts->tabstop; + vis_column = ((vis_column + tab_width) / tab_width) * tab_width; + return vis_column; +} + /* Convert libcpp's notion of a column (a 1-based char count) to the "visual column" (0-based column, respecting tabs), by reading the relevant line. @@ -77,11 +87,7 @@ get_visual_column (expanded_location exploc, location_t loc, } if (ch == '\t') - { - /* Round up to nearest tab stop. */ - const unsigned int tab_width = cpp_opts->tabstop; - vis_column = ((vis_column + tab_width) / tab_width) * tab_width; - } + vis_column = next_tab_stop (vis_column); else vis_column++; } @@ -93,54 +99,49 @@ get_visual_column (expanded_location exploc, location_t loc, return true; } -/* Does the given source line appear to contain a #if directive? - (or #ifdef/#ifndef). Ignore the possibility of it being inside a - comment, for simplicity. - Helper function for detect_preprocessor_logic. */ +/* Attempt to determine the first non-whitespace character in line LINE_NUM + of source line FILE. + + If this is possible, return true and write its "visual column" to + *FIRST_NWS. + Otherwise, return false, leaving *FIRST_NWS untouched. */ static bool -line_contains_hash_if (const char *file, int line_num) +get_first_nws_vis_column (const char *file, int line_num, + unsigned int *first_nws) { + gcc_assert (first_nws); + int line_len; const char *line = location_get_source_line (file, line_num, &line_len); if (!line) return false; + unsigned int vis_column = 0; + for (int i = 1; i < line_len; i++) + { + unsigned char ch = line[i - 1]; - int idx; - - /* Skip leading whitespace. */ - for (idx = 0; idx < line_len; idx++) - if (!ISSPACE (line[idx])) - break; - if (idx == line_len) - return false; - - /* Require a '#' character. */ - if (line[idx] != '#') - return false; - idx++; + if (!ISSPACE (ch)) + { + *first_nws = vis_column; + return true; + } - /* Skip whitespace. */ - while (idx < line_len) - { - if (!ISSPACE (line[idx])) - break; - idx++; + if (ch == '\t') + vis_column = next_tab_stop (vis_column); + else + vis_column++; } - /* Match #if/#ifdef/#ifndef. */ - if (idx + 2 <= line_len) - if (line[idx] == 'i') - if (line[idx + 1] == 'f') - return true; - + /* No non-whitespace characters found. */ return false; } - -/* Determine if there is preprocessor logic between +/* Determine if there is an unindent/outdent between BODY_EXPLOC and NEXT_STMT_EXPLOC, to ensure that we don't - issue a warning for cases like this: + issue a warning for cases like the following: + + (1) Preprocessor logic if (flagA) foo (); @@ -151,31 +152,47 @@ line_contains_hash_if (const char *file, int line_num) bar (); ^ NEXT_STMT_EXPLOC - despite "bar ();" being visually aligned below "foo ();" and - being (as far as the parser sees) the next token. + "bar ();" is visually aligned below "foo ();" and + is (as far as the parser sees) the next token, but + this isn't misleading to a human reader. - Return true if such logic is detected. */ + (2) Empty macro with bad indentation -static bool -detect_preprocessor_logic (expanded_location body_exploc, - expanded_location next_stmt_exploc) -{ - gcc_assert (next_stmt_exploc.file == body_exploc.file); - gcc_assert (next_stmt_exploc.line > body_exploc.line); + In the following, the + "if (i > 0)" + is poorly indented, and ought to be on the same column as + "engine_ref_debug(e, 0, -1)" + However, it is not misleadingly indented, due to the presence + of that macro. - if (next_stmt_exploc.line - body_exploc.line < 4) - return false; + #define engine_ref_debug(X, Y, Z) + + if (locked) + i = foo (0); + else + i = foo (1); + engine_ref_debug(e, 0, -1) + if (i > 0) + return 1; - /* Is there a #if/#ifdef/#ifndef directive somewhere in the lines - between the given locations? + Return true if such an unindent/outdent is detected. */ - This is something of a layering violation, but by necessity, - given the nature of what we're testing for. For example, - in theory we could be fooled by a #if within a comment, but - it's unlikely to matter. */ - for (int line = body_exploc.line + 1; line < next_stmt_exploc.line; line++) - if (line_contains_hash_if (body_exploc.file, line)) - return true; +static bool +detect_intervening_unindent (const char *file, + int body_line, + int next_stmt_line, + unsigned int vis_column) +{ + gcc_assert (file); + gcc_assert (next_stmt_line > body_line); + + for (int line = body_line + 1; line < next_stmt_line; line++) + { + unsigned int line_vis_column; + if (get_first_nws_vis_column (file, line, &line_vis_column)) + if (line_vis_column < vis_column) + return true; + } /* Not found. */ return false; @@ -467,9 +484,11 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo, if (body_vis_column <= guard_line_first_nws) return false; - /* Don't warn if there is multiline preprocessor logic between - the two statements. */ - if (detect_preprocessor_logic (body_exploc, next_stmt_exploc)) + /* Don't warn if there is an unindent between the two statements. */ + int vis_column = MIN (next_stmt_vis_column, body_vis_column); + if (detect_intervening_unindent (body_exploc.file, body_exploc.line, + next_stmt_exploc.line, + vis_column)) return false; /* Otherwise, they are visually aligned: issue a warning. */ diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c index 04500b7..7b499d4 100644 --- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c +++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c @@ -982,3 +982,75 @@ fn_41_b (void) if (!flagC) goto fail; } + +/* In the following, the + "if (i > 0)" + is poorly indented, and ought to be on the same column as + "engine_ref_debug(e, 0, -1)" + However, it is not misleadingly indented, due to the presence + of that macro. Verify that we do not emit a warning about it + not being guarded by the "else" clause above. + + Based on an example seen in OpenSSL 1.0.1, which was filed as + PR c/68187 in comment #1, though it's arguably a separate bug to + the one in comment #0. */ + +int +fn_42_a (int locked) +{ +#define engine_ref_debug(X, Y, Z) + + int i; + + if (locked) + i = foo (0); + else + i = foo (1); + engine_ref_debug(e, 0, -1) + if (i > 0) + return 1; + return 0; +#undef engine_ref_debug +} + +/* As above, but the empty macro is at the same indentation level. + This *is* misleading; verify that we do emit a warning about it. */ + +int +fn_42_b (int locked) +{ +#define engine_ref_debug(X, Y, Z) + + int i; + + if (locked) + i = foo (0); + else /* { dg-message "...this .else. clause" } */ + i = foo (1); + engine_ref_debug(e, 0, -1) + if (i > 0) /* { dg-warning "statement is indented" } */ + return 1; + return 0; +#undef engine_ref_debug +} + +/* As above, but where the body is a semicolon "hidden" by a preceding + comment, where the semicolon is not in the same column as the successor + "if" statement, but the empty macro expansion is at the same indentation + level as the guard. + This is poor indentation, but not misleading; verify that we don't emit a + warning about it. */ + +int +fn_42_c (int locked, int i) +{ +#define engine_ref_debug(X, Y, Z) + + if (locked) + /* blah */; + engine_ref_debug(e, 0, -1) + if (i > 0) + return 1; + return 0; +#undef engine_ref_debug +} -- 1.8.5.3