From 8ebca419e837774146ef77574580456107d7315b Mon Sep 17 00:00:00 2001 From: Patrick Palka Date: Sun, 2 Aug 2015 17:39:23 +0000 Subject: [PATCH] Improve -Wmisleading-indentation heuristics gcc/c-family/ChangeLog: * c-indentation.c (should_warn_for_misleading_indentation): Improve heuristics. gcc/testsuite/ChangeLog: * c-c++-common/Wmisleading-indentation.c: Add more tests. From-SVN: r226479 --- gcc/c-family/ChangeLog | 5 + gcc/c-family/c-indentation.c | 167 +++++++++++++++--- gcc/testsuite/ChangeLog | 4 + .../c-c++-common/Wmisleading-indentation.c | 166 +++++++++++++++++ 4 files changed, 322 insertions(+), 20 deletions(-) diff --git a/gcc/c-family/ChangeLog b/gcc/c-family/ChangeLog index bb74de9935a..02374d33f82 100644 --- a/gcc/c-family/ChangeLog +++ b/gcc/c-family/ChangeLog @@ -1,3 +1,8 @@ +2015-08-02 Patrick Palka + + * c-indentation.c (should_warn_for_misleading_indentation): + Improve heuristics. + 2015-08-02 Patrick Palka * c-indentation.c (get_visual_column): Add parameter first_nws, diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c index cdf0372aed3..fdfe0a93412 100644 --- a/gcc/c-family/c-indentation.c +++ b/gcc/c-family/c-indentation.c @@ -182,6 +182,7 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo, location_t body_loc = body_tinfo.location; location_t next_stmt_loc = next_tinfo.location; + enum cpp_ttype body_type = body_tinfo.type; enum cpp_ttype next_tok_type = next_tinfo.type; /* Don't attempt to compare the indentation of BODY_LOC and NEXT_STMT_LOC @@ -227,12 +228,33 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo, || next_tinfo.keyword == RID_ELSE) return false; + /* Likewise, if the body of the guard is a compound statement then control + flow is quite visually explicit regardless of the code's possibly poor + indentation, e.g. + + while (foo) <- GUARD + { <- BODY + bar (); + } + baz (); <- NEXT + + Things only get muddy when the body of the guard does not have + braces, e.g. + + if (foo) <- GUARD + bar (); <- BODY + baz (); <- NEXT + */ + if (body_type == CPP_OPEN_BRACE) + return false; + /* Don't warn here about spurious semicolons. */ if (next_tok_type == CPP_SEMICOLON) return false; expanded_location body_exploc = expand_location (body_loc); expanded_location next_stmt_exploc = expand_location (next_stmt_loc); + expanded_location guard_exploc = expand_location (guard_loc); /* They must be in the same file. */ if (next_stmt_exploc.file != body_exploc.file) @@ -250,13 +272,20 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo, if (flag) foo (); bar (); ^ WARN HERE + + if (flag) ; { + ^ WARN HERE + + if (flag) + ; { + ^ WARN HERE + Cases where we don't want to issue a warning: various_code (); if (flag) foo (); bar (); more_code (); ^ DON'T WARN HERE. */ if (next_stmt_exploc.line == body_exploc.line) { - expanded_location guard_exploc = expand_location (guard_loc); if (guard_exploc.file != body_exploc.file) return true; if (guard_exploc.line < body_exploc.line) @@ -304,14 +333,10 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo, bar (); ^ DON'T WARN HERE - if (flag) { - foo (); - } else - { - bar (); - } - baz (); - ^ DON'T WARN HERE + if (flag) + ; + foo (); + ^ DON'T WARN HERE */ if (next_stmt_exploc.line > body_exploc.line) { @@ -319,29 +344,84 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo, "visual column"... */ unsigned int next_stmt_vis_column; unsigned int body_vis_column; + unsigned int body_line_first_nws; /* If we can't determine it, don't issue a warning. This is sometimes the case for input files containing #line directives, and these are often for autogenerated sources (e.g. from .md files), where it's not clear that it's meaningful to look at indentation. */ if (!get_visual_column (next_stmt_exploc, &next_stmt_vis_column)) return false; - if (!get_visual_column (body_exploc, &body_vis_column)) + if (!get_visual_column (body_exploc, + &body_vis_column, + &body_line_first_nws)) return false; - if (next_stmt_vis_column == body_vis_column) + if ((body_type != CPP_SEMICOLON + && next_stmt_vis_column == body_vis_column) + /* As a special case handle the case where the body is a semicolon + that may be hidden by a preceding comment, e.g. */ + + // if (p) + // /* blah */; + // foo (1); + + /* by looking instead at the column of the first non-whitespace + character on the body line. */ + || (body_type == CPP_SEMICOLON + && body_exploc.line > guard_exploc.line + && body_line_first_nws != body_vis_column + && next_stmt_vis_column == body_line_first_nws)) { - /* Don't warn if they aren't aligned on the same column - as the guard itself (suggesting autogenerated code that - doesn't bother indenting at all). */ - expanded_location guard_exploc = expand_location (guard_loc); + /* Don't warn if they are aligned on the same column + as the guard itself (suggesting autogenerated code that doesn't + bother indenting at all). We consider the column of the first + non-whitespace character on the guard line instead of the column + of the actual guard token itself because it is more sensible. + Consider: + + if (p) { + foo (1); + } else // GUARD + foo (2); // BODY + foo (3); // NEXT + + and: + + if (p) + foo (1); + } else // GUARD + foo (2); // BODY + foo (3); // NEXT + + If we just used the column of the guard token, we would warn on + the first example and not warn on the second. But we want the + exact opposite to happen: to not warn on the first example (which + is probably autogenerated) and to warn on the second (whose + indentation is misleading). Using the column of the first + non-whitespace character on the guard line makes that + happen. */ unsigned int guard_vis_column; - if (!get_visual_column (guard_exploc, &guard_vis_column)) + unsigned int guard_line_first_nws; + if (!get_visual_column (guard_exploc, + &guard_vis_column, + &guard_line_first_nws)) return false; - if (guard_vis_column == body_vis_column) + + if (guard_line_first_nws == body_vis_column) return false; - /* PR 66220: Don't warn if the guarding statement is more - indented than the next/body stmts. */ - if (guard_vis_column > body_vis_column) + /* We may have something like: + + if (p) + { + foo (1); + } else // GUARD + foo (2); // BODY + foo (3); // NEXT + + in which case the columns are not aligned but the code is not + misleadingly indented. If the column of the body is less than + that of the guard line then don't warn. */ + if (body_vis_column < guard_line_first_nws) return false; /* Don't warn if there is multiline preprocessor logic between @@ -352,6 +432,53 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo, /* Otherwise, they are visually aligned: issue a warning. */ return true; } + + /* Also issue a warning for code having the form: + + if (flag); + foo (); + + while (flag); + { + ... + } + + for (...); + { + ... + } + + if (flag) + ; + else if (flag); + foo (); + + where the semicolon at the end of each guard is most likely spurious. + + But do not warn on: + + for (..); + foo (); + + where the next statement is aligned with the guard. + */ + if (body_type == CPP_SEMICOLON) + { + if (body_exploc.line == guard_exploc.line) + { + unsigned int guard_vis_column; + unsigned int guard_line_first_nws; + if (!get_visual_column (guard_exploc, + &guard_vis_column, + &guard_line_first_nws)) + return false; + + if (next_stmt_vis_column > guard_line_first_nws + || (next_tok_type == CPP_OPEN_BRACE + && next_stmt_vis_column == guard_vis_column)) + return true; + } + } } return false; diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index c018c14f323..82aa654a8bc 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,7 @@ +2015-08-02 Patrick Palka + + * c-c++-common/Wmisleading-indentation.c: Add more tests. + 2015-08-01 Michael Collison diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c index 443e3ddc30a..0d6d8d2cc4e 100644 --- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c +++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c @@ -691,3 +691,169 @@ fn_36 (void) } foo(6); /* We shouldn't warn here. */ } + +/* The following function contain code whose indentation is misleading, thus + we warn about it. */ + +void +fn_37 (void) +{ + int i; + +#define EMPTY +#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP; VAR++) + + while (flagA); /* { dg-message "3: ...this 'while' clause" } */ + foo (0); /* { dg-warning "statement is indented as if" } */ + + if (flagA) + ; + else if (flagA); /* { dg-message "8: ...this 'if' clause" } */ + foo (0); /* { dg-warning "statement is indented as if" } */ + while (flagA) /* { dg-message "3: ...this 'while' clause" } */ + /* blah */; + foo (0); /* { dg-warning "statement is indented as if" } */ + + if (flagA) + ; + else if (flagA) /* { dg-message "8: ...this 'if' clause" } */ + foo (1); + foo (2); /* { dg-warning "statement is indented as if" } */ + + if (flagA) + foo (1); + else if (flagA) /* { dg-message "8: ...this 'if' clause" } */ + foo (2); + foo (3); /* { dg-warning "statement is indented as if" } */ + + if (flagB) /* { dg-message "3: ...this 'if' clause" } */ + /* blah */; + { /* { dg-warning "statement is indented as if" } */ + foo (0); + } + + if (flagB) + ; + else; foo (0); /* { dg-warning "statement is indented as if" } */ + + if (flagC); foo (2); /* { dg-warning "statement is indented as if" } */ + + if (flagA) + ; /* blah */ { /* { dg-warning "statement is indented as if" } */ + foo (1); + } + + if (flagB) ; /* { dg-message "3: ...this 'if' clause" } */ + return; /* { dg-warning "statement is indented as if" } */ + + if (flagB) EMPTY; /* { dg-message "3: ...this 'if' clause" } */ + foo (1); /* { dg-warning "statement is indented as if" } */ + + for (i = 0; i < 10; i++); /* { dg-message "3: ...this 'for' clause" } */ + foo (2); /* { dg-warning "statement is indented as if" } */ + + FOR_EACH (i, 0, 10); + foo (2); /* { dg-warning "statement is indented as if" } */ + + FOR_EACH (i, 0, 10); + { /* { dg-warning "statement is indented as if" } */ + foo (3); + } + + FOR_EACH (i, 0, 10); + { /* { dg-warning "statement is indented as if" } */ + foo (3); + } + + while (i++); { /* { dg-warning "statement is indented as if" } */ + foo (3); + } + + if (i++); { /* { dg-warning "statement is indented as if" } */ + foo (3); + } + + if (flagA) { + foo (1); + } else /* { dg-message "5: ...this 'else' clause" } */ + if (flagB) + foo (2); + foo (3); /* { dg-warning "statement is indented as if" } */ + + if (flagA) + foo (1); + else if (flagB); /* { dg-message "8: ...this 'if' clause" } */ + foo (2); /* { dg-warning "statement is indented as if" } */ + +#undef EMPTY +#undef FOR_EACH +} + +/* The following function contains code whose indentation is not great but not + misleading, thus we don't warn. */ + +void +fn_38 (void) +{ + int i = 0; + + while (flagA) + ; + foo (0); + + if (flagB) + ; + { + foo (0); + } + + while (flagC); + foo (2); + + if (flagA) + while (flagC++); + else + foo (2); + + if (i) + while (i++ < 10000); + foo (5); + + if (i) while (i++ < 10000); + foo (5); + + if (flagA) { + foo (1); + } else + if (flagB) + foo (2); + foo (3); + + if (flagA) + { + foo (1); + } else + if (flagB) + foo (2); + foo (3); +} + +/* The following function contains good indentation which we definitely should + not warn about. */ + +void +fn_39 (void) +{ + if (flagA) + ; + if (flagB) + ; + + if (flagA) + if (flagB) + foo (0); + else + foo (1); + else + foo (2); +} -- 2.30.2