From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 113194 invoked by alias); 3 Mar 2016 14:58:52 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 113024 invoked by uid 89); 3 Mar 2016 14:58:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=1.0.1, visual, Match, cpp_opts X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 03 Mar 2016 14:58:48 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 52A177EBA0 for ; Thu, 3 Mar 2016 14:58:47 +0000 (UTC) Received: from c64.redhat.com (vpn-230-159.phx2.redhat.com [10.3.230.159]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u23EwjJ6002960; Thu, 3 Mar 2016 09:58:46 -0500 From: David Malcolm To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [PATCH 2/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #1) Date: Thu, 03 Mar 2016 14:58:00 -0000 Message-Id: <1457018483-26829-2-git-send-email-dmalcolm@redhat.com> In-Reply-To: <1457018483-26829-1-git-send-email-dmalcolm@redhat.com> References: <1457018483-26829-1-git-send-email-dmalcolm@redhat.com> X-IsSubscribed: yes X-SW-Source: 2016-03/txt/msg00261.txt.bz2 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