On Mon, 2015-11-02 at 16:41 -0700, Jeff Law wrote: > On 11/02/2015 12:35 PM, David Malcolm wrote: > > > > >> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c > >> index fff4862..2559a36 100644 > >> --- a/gdb/ada-lang.c > >> +++ b/gdb/ada-lang.c > >> @@ -11359,9 +11359,11 @@ ada_evaluate_subexp (struct type *expect_type, > >> struct expression *exp, > >> return value_zero (ada_aligned_type (type), lval_memory); > >> } > >> else > >> - arg1 = ada_value_struct_elt (arg1, &exp->elts[pc + 2].string, 0); > >> - arg1 = unwrap_value (arg1); > >> - return ada_to_fixed_value (arg1); > >> + { > >> + arg1 = ada_value_struct_elt (arg1, &exp->elts[pc + 2].string, 0); > >> + arg1 = unwrap_value (arg1); > >> + return ada_to_fixed_value (arg1); > >> + } > >> > >> case OP_TYPE: > >> /* The value is not supposed to be used. This is here to make it > > > > Interesting. It's not technically a bug, since the "if true" clause has > > an unconditional return, but it looks like a time-bomb to me. I'm happy > > that we warn for it. > Agreed. > > > > > > These three are all of the form: > > if (record_debug) > > fprint (...multiple lines...); > > return -1; > > > > It seems reasonable to me to complain about the indentation of the > > return -1; > Agreed. > > > > > >> diff --git a/sysdeps/ieee754/flt-32/k_rem_pio2f.c > >> b/sysdeps/ieee754/flt-32/k_rem_pio2f.c > >> index 0c7685c..a0d844c 100644 > >> --- a/sysdeps/ieee754/flt-32/k_rem_pio2f.c > >> +++ b/sysdeps/ieee754/flt-32/k_rem_pio2f.c > >> @@ -65,7 +65,8 @@ int __kernel_rem_pio2f(float *x, float *y, int e0, int > >> nx, int prec, const int32 > >> > >> /* compute q[0],q[1],...q[jk] */ > >> for (i=0;i<=jk;i++) { > >> - for(j=0,fw=0.0;j<=jx;j++) fw += x[j]*f[jx+i-j]; q[i] = fw; > >> + for(j=0,fw=0.0;j<=jx;j++) fw += x[j]*f[jx+i-j]; > >> + q[i] = fw; > >> } > >> > >> jz = jk; > > > > I think it's very reasonable to complain about the above code. > Agreed. > > > > > So if I've counted things right the tally is: > > * 5 dubious-looking though correct places, where it's reasonable to > > issue a warning > > * 5 places where there's a blank line between guarded and non-guarded > > stmt, where patch 1 of the kit would have suppressed the warning > > * one bug (PR 68187) > > > > I think we want the first kind of thing at -Wall, but I'm not so keen on > > the second kind at -Wall. Is there precedent for "levels" of a warning? > > (so e.g. pedantry level 1 at -Wall, level 2 at -Wextra, and have patch 1 > > be the difference between levels 1 and 2?) > > > > Sorry for harping on about patch 1; thanks again for posting this > No worries about harping. These are real world cases. > > And yes, there's definitely precedent for different levels of a warning. > If you wanted to add a patch to have different levels and select one > for -Wall, I'd look favorably on that.
The attached patch implements this idea. It's a revised version of: "[PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines" (https://gcc.gnu.org/ml/gcc-patches/2015-10/msg03225.html) (which wasn't approved, since you *did* want warnings for them), and of: "[PATCH 4/4] Add -Wmisleading-indentation to -Wall" (https://gcc.gnu.org/ml/gcc-patches/2015-10/msg03223.html) which you approved, but which Richi was unhappy with (I haven't committed it). The attached patch adds the idea of a strictness level to -Wmisleading-indentation, where -Wmisleading-indentation becomes equivalent to -Wmisleading-indentation=1 The heuristic from patch 1 becomes the difference between -Wmisleading-indentation=1 and -Wmisleading-indentation=2. It adds -Wmisleading-indentation=1 to -Wall, with -Wmisleading-indentation=2 available for users who want extra strictness. I think this gives us a big improvement in the signal:noise ratio of the "-Wall" form of the warning for real code (see e.g. the stats for gcc itself above), relative to the earlier form of the -Wall patch. Bootstrapped®rtested with x86_64-pc-linux-gnu. Adds 33 PASS results to g++.sum; adds 11 PASS results to gcc.sum. OK for trunk? gcc/c-family/ChangeLog: * c-indentation.c (line_is_blank_p): New function. (separated_by_blank_lines_p): New function. (should_warn_for_misleading_indentation): Add param strictness_level. Move early reject from warn_for_misleading_indentation to here. At strictness level 1, don't warn if the guarded and unguarded code are separated by one or more entirely blank lines. (warn_for_misleading_indentation): Pass value of warn_misleading_indentation to should_warn_for_misleading_indentation and move early rejection case for disabled aka level 0 to there. * c.opt (Wmisleading-indentation): Convert to an alias for... (Wmisleading-indentation=): ...this new version of -Wmisleading-indentation, which accepts an argument. Add level 1 to -Wall for C and C++. gcc/ChangeLog: * doc/invoke.texi (Warning Options): Add -Wmisleading-indentation=n. (-Wall): Add -Wmisleading-indentation=1 to the list. (-Wmisleading-indentation): Add -Wmisleading-indentation=n and description of levels. Update documentation to reflect being enabled by -Wall in C/C++. Provide an example showing the difference between levels 1 and 2. gcc/testsuite/ChangeLog: * c-c++-common/Wmisleading-indentation-in-Wall.c: New. * c-c++-common/Wmisleading-indentation-level-1.c: New. * c-c++-common/Wmisleading-indentation-level-2.c: New. * c-c++-common/Wmisleading-indentation.c (fn_40_implicit_level_1): New test function. --- gcc/c-family/c-indentation.c | 82 +++++++++++++++++++--- gcc/c-family/c.opt | 6 +- gcc/doc/invoke.texi | 31 +++++++- .../c-c++-common/Wmisleading-indentation-in-Wall.c | 30 ++++++++ .../c-c++-common/Wmisleading-indentation-level-1.c | 35 +++++++++ .../c-c++-common/Wmisleading-indentation-level-2.c | 34 +++++++++ .../c-c++-common/Wmisleading-indentation.c | 37 ++++++++++ 7 files changed, 243 insertions(+), 12 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/Wmisleading-indentation-in-Wall.c create mode 100644 gcc/testsuite/c-c++-common/Wmisleading-indentation-level-1.c create mode 100644 gcc/testsuite/c-c++-common/Wmisleading-indentation-level-2.c diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c index 638a9cf..ca7efdc 100644 --- a/gcc/c-family/c-indentation.c +++ b/gcc/c-family/c-indentation.c @@ -73,6 +73,42 @@ get_visual_column (expanded_location exploc, return true; } +/* Determine if the given source line of length LINE_LEN is entirely blank, + or contains one or more non-whitespace characters. */ + +static bool +line_is_blank_p (const char *line, int line_len) +{ + for (int i = 0; i < line_len; i++) + if (!ISSPACE (line[i])) + return false; + + return true; +} + +/* Helper function for should_warn_for_misleading_indentation. + Determines if there are one or more blank lines between the + given source lines. */ + +static bool +separated_by_blank_lines_p (const char *file, + int start_line, int end_line) +{ + gcc_assert (start_line < end_line); + for (int line_num = start_line; line_num < end_line; line_num++) + { + int line_len; + const char *line = location_get_source_line (file, line_num, &line_len); + if (!line) + return false; + + if (line_is_blank_p (line, line_len)) + return true; + } + + return false; +} + /* 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. @@ -166,10 +202,17 @@ detect_preprocessor_logic (expanded_location body_exploc, description of that function below. */ static bool -should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo, +should_warn_for_misleading_indentation (int strictness_level, + const token_indent_info &guard_tinfo, const token_indent_info &body_tinfo, const token_indent_info &next_tinfo) { + /* Early reject for the case where -Wmisleading-indentation is disabled, + to avoid doing work only to have the warning suppressed inside the + diagnostic machinery. */ + if (!strictness_level) + return false; + location_t guard_loc = guard_tinfo.location; location_t body_loc = body_tinfo.location; location_t next_stmt_loc = next_tinfo.location; @@ -329,6 +372,15 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo, ; foo (); ^ DON'T WARN HERE + + Cases where we may want to issue a warning: + + if (flag) + foo (); + + bar (); + ^ blank line between guarded and unguarded code + Warn here at level 2, but not at level 1. */ if (next_stmt_exploc.line > body_exploc.line) { @@ -354,6 +406,23 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo, &guard_line_first_nws)) return false; + /* At level 1, don't warn if the guarded and unguarded code are + separated by one or more entirely blank lines, e.g.: + switch (arg) + { + case 0: + if (flagA) + foo (0); + + break; + ^ DON'T WARN HERE + } + since this is arguably not misleading. */ + if (strictness_level < 2) + if (separated_by_blank_lines_p (body_exploc.file, body_exploc.line, + next_stmt_exploc.line)) + return false; + 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 @@ -522,17 +591,12 @@ warn_for_misleading_indentation (const token_indent_info &guard_tinfo, const token_indent_info &body_tinfo, const token_indent_info &next_tinfo) { - /* Early reject for the case where -Wmisleading-indentation is disabled, - to avoid doing work only to have the warning suppressed inside the - diagnostic machinery. */ - if (!warn_misleading_indentation) - return; - - if (should_warn_for_misleading_indentation (guard_tinfo, + if (should_warn_for_misleading_indentation (warn_misleading_indentation, + guard_tinfo, body_tinfo, next_tinfo)) { - if (warning_at (next_tinfo.location, OPT_Wmisleading_indentation, + if (warning_at (next_tinfo.location, OPT_Wmisleading_indentation_, "statement is indented as if it were guarded by...")) inform (guard_tinfo.location, "...this %qs clause, but it is not", diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index aafd802..3cb190d 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -562,7 +562,11 @@ C ObjC C++ ObjC++ Var(warn_memset_transposed_args) Warning LangEnabledBy(C ObjC Warn about suspicious calls to memset where the third argument is constant literal zero and the second is not. Wmisleading-indentation -C C++ Common Var(warn_misleading_indentation) Warning +C C++ Common Warning Alias(Wmisleading-indentation=, 1, 0) +Warn when the indentation of the code does not reflect the block structure. + +Wmisleading-indentation= +C C++ Common Joined RejectNegative UInteger Var(warn_misleading_indentation) Warning LangEnabledBy(C C++,Wall, 1, 0) Warn when the indentation of the code does not reflect the block structure. Wmissing-braces diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 5256031..25a11ef 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -264,7 +264,7 @@ Objective-C and Objective-C++ Dialects}. -Winvalid-pch -Wlarger-than=@var{len} @gol -Wlogical-op -Wlogical-not-parentheses -Wlong-long @gol -Wmain -Wmaybe-uninitialized -Wmemset-transposed-args @gol --Wmisleading-indentation -Wmissing-braces @gol +-Wmisleading-indentation -Wmisleading-indentation=@var{n} -Wmissing-braces @gol -Wmissing-field-initializers -Wmissing-include-dirs @gol -Wno-multichar -Wnonnull -Wnormalized=@r{[}none@r{|}id@r{|}nfc@r{|}nfkc@r{]} @gol -Wnull-dereference -Wodr -Wno-overflow -Wopenmp-simd @gol @@ -3551,6 +3551,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wformat @gol -Wmain @r{(only for C/ObjC and unless} @option{-ffreestanding}@r{)} @gol -Wmaybe-uninitialized @gol +-Wmisleading-indentation=1 @r{(only for C/C++)} @gol -Wmissing-braces @r{(only for C/ObjC)} @gol -Wnonnull @gol -Wopenmp-simd @gol @@ -3889,6 +3890,7 @@ is enabled by default in C++ and is enabled by either @option{-Wall} or @option{-Wpedantic}. @item -Wmisleading-indentation @r{(C and C++ only)} +@itemx -Wmisleading-indentation=@var{n} @opindex Wmisleading-indentation @opindex Wno-misleading-indentation Warn when the indentation of the code does not reflect the block structure. @@ -3896,7 +3898,12 @@ Specifically, a warning is issued for @code{if}, @code{else}, @code{while}, and @code{for} clauses with a guarded statement that does not use braces, followed by an unguarded statement with the same indentation. -This warning is disabled by default. +This warning is disabled by default. The warning can optionally accept a +level (either 1 or 2), for specifying increasing levels of strictness; +@option{-Wmisleading-indentation} (with no level) is equivalent to +@option{-Wmisleading-indentation=1}. + +@option{-Wmisleading-indentation=1} is enabled by @option{-Wall} in C and C++. In the following example, the call to ``bar'' is misleadingly indented as if it were guarded by the ``if'' conditional. @@ -3907,6 +3914,26 @@ if it were guarded by the ``if'' conditional. bar (); /* Gotcha: this is not guarded by the "if". */ @end smallexample +A warning will be issued for this at @option{-Wmisleading-indentation=1} +and above. + +If there is a blank line between the guarded code and the unguarded code, +such as in the following: + +@smallexample + @{ + foo (0); + + if (some_condition) /* these two lines are not indented enough. */ + foo (1); + + foo (2); /* this lines up with "foo (1);" but is not guarded by the "if". */ + @} +@end smallexample + +then a warning will be issued at @option{-Wmisleading-indentation=2}, +but not at @option{-Wmisleading-indentation=1}. + In the case of mixed tabs and spaces, the warning uses the @option{-ftabstop=} option to determine if the statements line up (defaulting to 8). diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-in-Wall.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-in-Wall.c new file mode 100644 index 0000000..a39e39a --- /dev/null +++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-in-Wall.c @@ -0,0 +1,30 @@ +/* { dg-options "-Wall" } */ +/* { dg-do compile } */ + +/* Verify that -Wmisleading-indentation is enabled by -Wall. */ + +int +fn_1 (int flag) +{ + int x = 4, y = 5; + if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" } */ + x = 3; + y = 2; /* { dg-warning "statement is indented as if it were guarded by..." } */ + return x * y; +} + +/* Ensure that we can disable the warning. */ + +int +fn_32 (int flag) +{ + int x = 4, y = 5; +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wmisleading-indentation" + if (flag) + x = 3; + y = 2; +#pragma GCC diagnostic pop + + return x * y; +} diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-level-1.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-level-1.c new file mode 100644 index 0000000..345e324 --- /dev/null +++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-level-1.c @@ -0,0 +1,35 @@ +/* { dg-options "-Wmisleading-indentation=1" } */ +/* { dg-do compile } */ + +extern int foo (int); +extern int flagA; + +/* At -Wmisleading-indentation=1 we shouldn't emit warnings for this + function. Compare with + fn_40_implicit_level_1 in Wmisleading-indentation.c and + fn_40_level_2 in Wmisleading-indentation-level-2.c. */ + +void +fn_40_level_1 (int arg) +{ +if (flagA) + foo (0); + + foo (1); + + + if (flagA) + foo (0); + + foo (1); + + + switch (arg) + { + case 0: + if (flagA) + foo (0); + + break; + } +} diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-level-2.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-level-2.c new file mode 100644 index 0000000..d4c51ea --- /dev/null +++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-level-2.c @@ -0,0 +1,34 @@ +/* { dg-options "-Wmisleading-indentation=2" } */ +/* { dg-do compile } */ + +extern int foo (int); +extern int flagA; + +/* Compare with fn_40_implicit_level_1 in Wmisleading-indentation.c and + fn_40_level_1 in Wmisleading-indentation-level-1.c. + Verify that we emit warnings at level 2 for this function. */ + +void +fn_40_level_2 (int arg) +{ +if (flagA) /* { dg-message "1: ...this 'if' clause" } */ + foo (0); + + foo (1); /* { dg-warning "statement is indented as if" } */ + + + if (flagA) /* { dg-message "3: ...this 'if' clause" } */ + foo (0); + + foo (1); /* { dg-warning "statement is indented as if" } */ + + + switch (arg) + { + case 0: + if (flagA) /* { dg-message "6: ...this 'if' clause" } */ + foo (0); + + break; /* { dg-warning "statement is indented as if" } */ + } +} diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c index a3f5acd..0e39906 100644 --- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c +++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c @@ -891,3 +891,40 @@ fn_39 (void) i++); foo (i); } + +/* The following function contains examples of bad indentation that's + arguably not misleading, due to a blank line between the guarded and the + non-guarded code. Some of the blank lines deliberately contain + redundant whitespace, to verify that this is handled. + Based on examples seen in gcc/fortran/io.c, gcc/function.c, and + gcc/regrename.c. + + At -Wmisleading-indentation (implying level 1) we shouldn't emit + warnings for this function. Compare with + fn_40_level_1 in Wmisleading-indentation-level-1.c and + fn_40_level_2 in Wmisleading-indentation-level-2.c. */ + +void +fn_40_implicit_level_1 (int arg) +{ +if (flagA) + foo (0); + + foo (1); + + + if (flagA) + foo (0); + + foo (1); + + + switch (arg) + { + case 0: + if (flagA) + foo (0); + + break; + } +} -- 1.8.5.3