* [PATCH 4/4] Add -Wmisleading-indentation to -Wall
2015-10-29 16:32 [PATCH 0/4] -Wmisleading-indentation David Malcolm
2015-10-29 16:32 ` [PATCH 3/4] Fix misleading indentation in tree-nested.c David Malcolm
@ 2015-10-29 16:32 ` David Malcolm
2015-10-29 17:42 ` Jeff Law
2015-10-29 16:32 ` [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines David Malcolm
` (2 subsequent siblings)
4 siblings, 1 reply; 44+ messages in thread
From: David Malcolm @ 2015-10-29 16:32 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
Our documentation describes -Wall as enabling "all the warnings about
constructions that some users consider questionable, and that are easy to avoid
(or modify to prevent the warning), even in conjunction with macros."
I believe that -Wmisleading-indentation meets these criteria, and is
likely to be of benefit to users who may not read release notes; it
warns for indentation that's misleading, but not for indentation
that's merely bad: the former are places where a user will likely
want to fix the code.
The fix is usually easy and obvious: fix the misleadingly-indented
code. If that isn't an option for some reason, pragmas can be used to
turn off the warning for a particular fragment of code:
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wmisleading-indentation"
if (flag)
x = 3;
y = 2;
#pragma GCC diagnostic pop
-Wmisleading-indentation has been tested with a variety of indentation
styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c)
and on a variety of real-world projects. For example, in:
https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html
Patrick reports:
"Tested by building the linux, git, vim, sqlite and gdb-binutils sources
with -Wmisleading-indentation."
With the tweak earlier in this kit I believe we now have a good
enough signal:noise ratio for this warning to be widely used; hence this
patch adds the warning to -Wall.
Bootstrapped®rtested with x86_64-pc-linux-gnu.
OK for trunk?
gcc/c-family/ChangeLog:
* c.opt (Wmisleading-indentation): Add to -Wall for C and C++.
gcc/ChangeLog:
* doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the
list.
(-Wmisleading-indentation): Update documentation to reflect
being enabled by -Wall in C/C++.
---
gcc/c-family/c.opt | 2 +-
gcc/doc/invoke.texi | 5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index e573254..d603511 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -562,7 +562,7 @@ 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 Var(warn_misleading_indentation) Warning LangEnabledBy(C C++,Wall)
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 5665315..abbbe5f 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -3531,6 +3531,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 @r{(only for C/C++)} @gol
-Wmissing-braces @r{(only for C/ObjC)} @gol
-Wnonnull @gol
-Wopenmp-simd @gol
@@ -3876,8 +3877,6 @@ 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.
-
In the following example, the call to ``bar'' is misleadingly indented as
if it were guarded by the ``if'' conditional.
@@ -3907,6 +3906,8 @@ The warning is not issued after a @code{#line} directive, since this
typically indicates autogenerated code, and no assumptions can be made
about the layout of the file that the directive references.
+This warning is enabled by @option{-Wall} in C and C++.
+
@item -Wmissing-braces
@opindex Wmissing-braces
@opindex Wno-missing-braces
--
1.8.5.3
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 3/4] Fix misleading indentation in tree-nested.c
2015-10-29 16:32 [PATCH 0/4] -Wmisleading-indentation David Malcolm
@ 2015-10-29 16:32 ` David Malcolm
2015-10-29 17:36 ` Jeff Law
2015-10-29 16:32 ` [PATCH 4/4] Add -Wmisleading-indentation to -Wall David Malcolm
` (3 subsequent siblings)
4 siblings, 1 reply; 44+ messages in thread
From: David Malcolm @ 2015-10-29 16:32 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
tree-nested.c has this code:
2333 for (c = gimple_omp_taskreg_clauses (stmt);
2334 c;
2335 c = OMP_CLAUSE_CHAIN (c))
2336 if ((OMP_CLAUSE_CODE (c) == OMP_CLAUSE_FIRSTPRIVATE
2337 || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_SHARED)
2338 && OMP_CLAUSE_DECL (c) == decl)
2339 break;
2340 if (c == NULL && gimple_code (stmt) != GIMPLE_OMP_TARGET)
2341 {
which leads to this warning from -Wmisleading-indentation
(justifiably, in my opinion):
../../../src/gcc/tree-nested.c: In function âtree_node* convert_tramp_reference_stmt(gimple_stmt_iterator*, bool*, walk_stmt_info* â:
../../../src/gcc/tree-nested.c:2340:8: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation]
if (c == NULL && gimple_code (stmt) != GIMPLE_OMP_TARGET)
^
../../../src/gcc/tree-nested.c:2333:6: note: ...this âforâ clause, but it is not
for (c = gimple_omp_taskreg_clauses (stmt);
^
This patch fixes the indentation of lines 2340-2360 to make it clear
that they're not part of the "for" loop.
gcc/ChangeLog:
* tree-nested.c (convert_tramp_reference_stmt): Fix indentation.
---
gcc/tree-nested.c | 42 +++++++++++++++++++++---------------------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/gcc/tree-nested.c b/gcc/tree-nested.c
index c04c429..a5435f9 100644
--- a/gcc/tree-nested.c
+++ b/gcc/tree-nested.c
@@ -2337,27 +2337,27 @@ convert_tramp_reference_stmt (gimple_stmt_iterator *gsi, bool *handled_ops_p,
|| OMP_CLAUSE_CODE (c) == OMP_CLAUSE_SHARED)
&& OMP_CLAUSE_DECL (c) == decl)
break;
- if (c == NULL && gimple_code (stmt) != GIMPLE_OMP_TARGET)
- {
- c = build_omp_clause (gimple_location (stmt),
- i ? OMP_CLAUSE_FIRSTPRIVATE
- : OMP_CLAUSE_SHARED);
- OMP_CLAUSE_DECL (c) = decl;
- OMP_CLAUSE_CHAIN (c) = gimple_omp_taskreg_clauses (stmt);
- gimple_omp_taskreg_set_clauses (stmt, c);
- }
- else if (c == NULL)
- {
- c = build_omp_clause (gimple_location (stmt),
- OMP_CLAUSE_MAP);
- OMP_CLAUSE_DECL (c) = decl;
- OMP_CLAUSE_SET_MAP_KIND (c,
- i ? GOMP_MAP_TO : GOMP_MAP_TOFROM);
- OMP_CLAUSE_SIZE (c) = DECL_SIZE_UNIT (decl);
- OMP_CLAUSE_CHAIN (c) = gimple_omp_target_clauses (stmt);
- gimple_omp_target_set_clauses (as_a <gomp_target *> (stmt),
- c);
- }
+ if (c == NULL && gimple_code (stmt) != GIMPLE_OMP_TARGET)
+ {
+ c = build_omp_clause (gimple_location (stmt),
+ i ? OMP_CLAUSE_FIRSTPRIVATE
+ : OMP_CLAUSE_SHARED);
+ OMP_CLAUSE_DECL (c) = decl;
+ OMP_CLAUSE_CHAIN (c) = gimple_omp_taskreg_clauses (stmt);
+ gimple_omp_taskreg_set_clauses (stmt, c);
+ }
+ else if (c == NULL)
+ {
+ c = build_omp_clause (gimple_location (stmt),
+ OMP_CLAUSE_MAP);
+ OMP_CLAUSE_DECL (c) = decl;
+ OMP_CLAUSE_SET_MAP_KIND (c,
+ i ? GOMP_MAP_TO : GOMP_MAP_TOFROM);
+ OMP_CLAUSE_SIZE (c) = DECL_SIZE_UNIT (decl);
+ OMP_CLAUSE_CHAIN (c) = gimple_omp_target_clauses (stmt);
+ gimple_omp_target_set_clauses (as_a <gomp_target *> (stmt),
+ c);
+ }
}
info->new_local_var_chain = save_local_var_chain;
info->static_chain_added |= save_static_chain_added;
--
1.8.5.3
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 0/4] -Wmisleading-indentation
@ 2015-10-29 16:32 David Malcolm
2015-10-29 16:32 ` [PATCH 3/4] Fix misleading indentation in tree-nested.c David Malcolm
` (4 more replies)
0 siblings, 5 replies; 44+ messages in thread
From: David Malcolm @ 2015-10-29 16:32 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
The following patches:
* tweak -Wmisleading-indentation to suppress a kind of false
positive that occurred in three places in gcc's source
tree, and
* fix the two remaining truly misleadingly-indented places
where the warning fires
* adds -Wmisleading-indentation to -Wall
Bootstrapped®rtested the combination of patches with
x86_64-pc-linux-gnu; config-list.mk build using
gcc6 with -Wmisleading-indentation enabled is in-progress.
OK for trunk?
David Malcolm (4):
-Wmisleading-indentation: don't warn in presence of entirely blank
lines
Fix misleading indentation in tree-ssa-loop-unswitch.c
Fix misleading indentation in tree-nested.c
Add -Wmisleading-indentation to -Wall
gcc/c-family/c-indentation.c | 58 ++++++++++++++++++++++
gcc/c-family/c.opt | 2 +-
gcc/doc/invoke.texi | 5 +-
.../c-c++-common/Wmisleading-indentation.c | 32 ++++++++++++
gcc/tree-nested.c | 42 ++++++++--------
gcc/tree-ssa-loop-unswitch.c | 2 +-
6 files changed, 116 insertions(+), 25 deletions(-)
--
1.8.5.3
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 2/4] Fix misleading indentation in tree-ssa-loop-unswitch.c
2015-10-29 16:32 [PATCH 0/4] -Wmisleading-indentation David Malcolm
` (2 preceding siblings ...)
2015-10-29 16:32 ` [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines David Malcolm
@ 2015-10-29 16:32 ` David Malcolm
2015-10-29 17:34 ` Jeff Law
2015-10-30 5:42 ` [PATCH 0/4] -Wmisleading-indentation Andi Kleen
4 siblings, 1 reply; 44+ messages in thread
From: David Malcolm @ 2015-10-29 16:32 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
tree-ssa-loop-unswitch.c has this bad indentation at line 452:
449 if (dump_file && (dump_flags & TDF_DETAILS))
450 fprintf (dump_file, ";; Not unswitching, loop is not expected"
451 " to iterate\n");
452 return false;
which leads to this warning from -Wmisleading-indentation (justifiably,
in my opinion):
../../../src/gcc/tree-ssa-loop-unswitch.c: In function âbool tree_unswitch_outer_loop(loop*)â:
../../../src/gcc/tree-ssa-loop-unswitch.c:452:2: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation]
return false;
^
../../../src/gcc/tree-ssa-loop-unswitch.c:449:7: note: ...this âifâ clause, but it is not
if (dump_file && (dump_flags & TDF_DETAILS))
^
This patch fixes the indentation of the "return false;"
gcc/ChangeLog:
* tree-ssa-loop-unswitch.c (tree_unswitch_outer_loop): Fix
indentation.
---
gcc/tree-ssa-loop-unswitch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/tree-ssa-loop-unswitch.c b/gcc/tree-ssa-loop-unswitch.c
index 2edc000..7a287da 100644
--- a/gcc/tree-ssa-loop-unswitch.c
+++ b/gcc/tree-ssa-loop-unswitch.c
@@ -449,7 +449,7 @@ tree_unswitch_outer_loop (struct loop *loop)
if (dump_file && (dump_flags & TDF_DETAILS))
fprintf (dump_file, ";; Not unswitching, loop is not expected"
" to iterate\n");
- return false;
+ return false;
}
guard = find_loop_guard (loop);
--
1.8.5.3
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines
2015-10-29 16:32 [PATCH 0/4] -Wmisleading-indentation David Malcolm
2015-10-29 16:32 ` [PATCH 3/4] Fix misleading indentation in tree-nested.c David Malcolm
2015-10-29 16:32 ` [PATCH 4/4] Add -Wmisleading-indentation to -Wall David Malcolm
@ 2015-10-29 16:32 ` David Malcolm
2015-10-29 17:33 ` Jeff Law
` (2 more replies)
2015-10-29 16:32 ` [PATCH 2/4] Fix misleading indentation in tree-ssa-loop-unswitch.c David Malcolm
2015-10-30 5:42 ` [PATCH 0/4] -Wmisleading-indentation Andi Kleen
4 siblings, 3 replies; 44+ messages in thread
From: David Malcolm @ 2015-10-29 16:32 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
Attempting to bootstrap gcc with -Wmisleading-indentation enabled I ran
into a few failures where the indentation, although bad, was arguably
not misleading.
In regrename.c:scan_rtx_address:
1308 case PRE_MODIFY:
1309 /* If the target doesn't claim to handle autoinc, this must be
1310 something special, like a stack push. Kill this chain. */
1311 if (!AUTO_INC_DEC)
1312 action = mark_all_read;
1313
1314 break;
^ this is indented at the same level as the "action =" code,
but clearly isn't guarded by the if () at line 1311.
In gcc/fortran/io.c:gfc_match_open:
1997 {
1998 static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", NULL };
1999
2000 if (!is_char_type ("DELIM", open->delim))
2001 goto cleanup;
2002
2003 if (!compare_to_allowed_values ("DELIM", delim, NULL, NULL,
2004 open->delim->value.character.string,
2005 "OPEN", warn))
^ this is indented with the "goto cleanup;" due to
lines 2000-2001 not being indented enough, but
line 2003 clearly isn't guarded by the
"if (!is_char_type" conditional.
In gcc/function.c:locate_and_pad_parm:
4118 locate->slot_offset.constant = -initial_offset_ptr->constant;
4119 if (initial_offset_ptr->var)
4120 locate->slot_offset.var = size_binop (MINUS_EXPR, ssize_int (0),
4121 initial_offset_ptr->var);
4122
4123 {
4124 tree s2 = sizetree;
4125 if (where_pad != none
4126 && (!tree_fits_uhwi_p (sizetree)
4127 || (tree_to_uhwi (sizetree) * BITS_PER_UNIT) % round_boundary))
4128 s2 = round_up (s2, round_boundary / BITS_PER_UNIT);
4129 SUB_PARM_SIZE (locate->slot_offset, s2);
4130 }
^ this block is not guarded by the
"if (initial_offset_ptr->var)"
and the whitespace line (4122) is likely to make a
human reader of the code read it that way also.
In each case, a blank line separated the guarded code from followup code
that wasn't guarded, and to my eyes, the blank line makes the meaning of
the badly-indented code sufficiently clear that it seems unjustified to
issue a -Wmisleading-indentation warning.
The attached patch suppresses the warning for such a case.
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): Don't warn if the
guarded and unguarded code are separated by one or more entirely
blank lines.
gcc/testsuite/ChangeLog:
* c-c++-common/Wmisleading-indentation.c (fn_40): New function.
---
gcc/c-family/c-indentation.c | 58 ++++++++++++++++++++++
.../c-c++-common/Wmisleading-indentation.c | 32 ++++++++++++
2 files changed, 90 insertions(+)
diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index 5b119f7..d9d4380 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -77,6 +77,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.
@@ -333,6 +369,12 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
;
foo ();
^ DON'T WARN HERE
+
+ if (flag)
+ foo ();
+
+ bar ();
+ ^ DON'T WARN HERE: blank line between guarded and unguarded code
*/
if (next_stmt_exploc.line > body_exploc.line)
{
@@ -358,6 +400,22 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
&guard_line_first_nws))
return false;
+ /* 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 (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
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
index a3f5acd..669d33c 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
@@ -891,3 +891,35 @@ fn_39 (void)
i++);
foo (i);
}
+
+/* The following function contains examples of bad indentation that's 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. */
+
+void
+fn_40 (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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines
2015-10-29 16:32 ` [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines David Malcolm
@ 2015-10-29 17:33 ` Jeff Law
2015-10-29 17:38 ` Patrick Palka
2015-10-29 17:59 ` AW: " bernds_cb1
2 siblings, 0 replies; 44+ messages in thread
From: Jeff Law @ 2015-10-29 17:33 UTC (permalink / raw)
To: David Malcolm, gcc-patches
On 10/29/2015 10:49 AM, David Malcolm wrote:
> Attempting to bootstrap gcc with -Wmisleading-indentation enabled I ran
> into a few failures where the indentation, although bad, was arguably
> not misleading.
>
> In regrename.c:scan_rtx_address:
>
> 1308 case PRE_MODIFY:
> 1309 /* If the target doesn't claim to handle autoinc, this must be
> 1310 something special, like a stack push. Kill this chain. */
> 1311 if (!AUTO_INC_DEC)
> 1312 action = mark_all_read;
> 1313
> 1314 break;
> ^ this is indented at the same level as the "action =" code,
> but clearly isn't guarded by the if () at line 1311.
>
> In gcc/fortran/io.c:gfc_match_open:
>
> 1997 {
> 1998 static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", NULL };
> 1999
> 2000 if (!is_char_type ("DELIM", open->delim))
> 2001 goto cleanup;
> 2002
> 2003 if (!compare_to_allowed_values ("DELIM", delim, NULL, NULL,
> 2004 open->delim->value.character.string,
> 2005 "OPEN", warn))
> ^ this is indented with the "goto cleanup;" due to
> lines 2000-2001 not being indented enough, but
> line 2003 clearly isn't guarded by the
> "if (!is_char_type" conditional.
>
> In gcc/function.c:locate_and_pad_parm:
>
> 4118 locate->slot_offset.constant = -initial_offset_ptr->constant;
> 4119 if (initial_offset_ptr->var)
> 4120 locate->slot_offset.var = size_binop (MINUS_EXPR, ssize_int (0),
> 4121 initial_offset_ptr->var);
> 4122
> 4123 {
> 4124 tree s2 = sizetree;
> 4125 if (where_pad != none
> 4126 && (!tree_fits_uhwi_p (sizetree)
> 4127 || (tree_to_uhwi (sizetree) * BITS_PER_UNIT) % round_boundary))
> 4128 s2 = round_up (s2, round_boundary / BITS_PER_UNIT);
> 4129 SUB_PARM_SIZE (locate->slot_offset, s2);
> 4130 }
> ^ this block is not guarded by the
> "if (initial_offset_ptr->var)"
> and the whitespace line (4122) is likely to make a
> human reader of the code read it that way also.
>
> In each case, a blank line separated the guarded code from followup code
> that wasn't guarded, and to my eyes, the blank line makes the meaning of
> the badly-indented code sufficiently clear that it seems unjustified to
> issue a -Wmisleading-indentation warning.
I would argue that each of these does represent misleading indentation
and that the warning is warranted for each. Perhaps they aren't as bad
as prior cases, but I'd still consider them mis-leading.
Even more so if you have to work in codebases with differing styles
(particularly where the open curly goes).
Jeff
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/4] Fix misleading indentation in tree-ssa-loop-unswitch.c
2015-10-29 16:32 ` [PATCH 2/4] Fix misleading indentation in tree-ssa-loop-unswitch.c David Malcolm
@ 2015-10-29 17:34 ` Jeff Law
0 siblings, 0 replies; 44+ messages in thread
From: Jeff Law @ 2015-10-29 17:34 UTC (permalink / raw)
To: David Malcolm, gcc-patches
On 10/29/2015 10:49 AM, David Malcolm wrote:
> tree-ssa-loop-unswitch.c has this bad indentation at line 452:
>
> 449 if (dump_file && (dump_flags & TDF_DETAILS))
> 450 fprintf (dump_file, ";; Not unswitching, loop is not expected"
> 451 " to iterate\n");
> 452 return false;
>
> which leads to this warning from -Wmisleading-indentation (justifiably,
> in my opinion):
>
> ../../../src/gcc/tree-ssa-loop-unswitch.c: In function âbool tree_unswitch_outer_loop(loop*)â:
> ../../../src/gcc/tree-ssa-loop-unswitch.c:452:2: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation]
> return false;
> ^
> ../../../src/gcc/tree-ssa-loop-unswitch.c:449:7: note: ...this âifâ clause, but it is not
> if (dump_file && (dump_flags & TDF_DETAILS))
> ^
>
> This patch fixes the indentation of the "return false;"
>
> gcc/ChangeLog:
> * tree-ssa-loop-unswitch.c (tree_unswitch_outer_loop): Fix
> indentation.
OK.
jeff
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 3/4] Fix misleading indentation in tree-nested.c
2015-10-29 16:32 ` [PATCH 3/4] Fix misleading indentation in tree-nested.c David Malcolm
@ 2015-10-29 17:36 ` Jeff Law
0 siblings, 0 replies; 44+ messages in thread
From: Jeff Law @ 2015-10-29 17:36 UTC (permalink / raw)
To: David Malcolm, gcc-patches
On 10/29/2015 10:49 AM, David Malcolm wrote:
> tree-nested.c has this code:
>
> 2333 for (c = gimple_omp_taskreg_clauses (stmt);
> 2334 c;
> 2335 c = OMP_CLAUSE_CHAIN (c))
> 2336 if ((OMP_CLAUSE_CODE (c) == OMP_CLAUSE_FIRSTPRIVATE
> 2337 || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_SHARED)
> 2338 && OMP_CLAUSE_DECL (c) == decl)
> 2339 break;
> 2340 if (c == NULL && gimple_code (stmt) != GIMPLE_OMP_TARGET)
> 2341 {
>
> which leads to this warning from -Wmisleading-indentation
> (justifiably, in my opinion):
>
> ../../../src/gcc/tree-nested.c: In function âtree_node* convert_tramp_reference_stmt(gimple_stmt_iterator*, bool*, walk_stmt_info* â:
> ../../../src/gcc/tree-nested.c:2340:8: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation]
> if (c == NULL && gimple_code (stmt) != GIMPLE_OMP_TARGET)
> ^
> ../../../src/gcc/tree-nested.c:2333:6: note: ...this âforâ clause, but it is not
> for (c = gimple_omp_taskreg_clauses (stmt);
> ^
>
> This patch fixes the indentation of lines 2340-2360 to make it clear
> that they're not part of the "for" loop.
>
> gcc/ChangeLog:
> * tree-nested.c (convert_tramp_reference_stmt): Fix indentation.
OK.
jeff
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines
2015-10-29 16:32 ` [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines David Malcolm
2015-10-29 17:33 ` Jeff Law
@ 2015-10-29 17:38 ` Patrick Palka
2015-10-29 17:44 ` Patrick Palka
2015-10-29 18:13 ` David Malcolm
2015-10-29 17:59 ` AW: " bernds_cb1
2 siblings, 2 replies; 44+ messages in thread
From: Patrick Palka @ 2015-10-29 17:38 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches
On Thu, Oct 29, 2015 at 12:49 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> Attempting to bootstrap gcc with -Wmisleading-indentation enabled I ran
> into a few failures where the indentation, although bad, was arguably
> not misleading.
>
> In regrename.c:scan_rtx_address:
>
> 1308 case PRE_MODIFY:
> 1309 /* If the target doesn't claim to handle autoinc, this must be
> 1310 something special, like a stack push. Kill this chain. */
> 1311 if (!AUTO_INC_DEC)
> 1312 action = mark_all_read;
> 1313
> 1314 break;
> ^ this is indented at the same level as the "action =" code,
> but clearly isn't guarded by the if () at line 1311.
>
> In gcc/fortran/io.c:gfc_match_open:
>
> 1997 {
> 1998 static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", NULL };
> 1999
> 2000 if (!is_char_type ("DELIM", open->delim))
> 2001 goto cleanup;
> 2002
> 2003 if (!compare_to_allowed_values ("DELIM", delim, NULL, NULL,
> 2004 open->delim->value.character.string,
> 2005 "OPEN", warn))
> ^ this is indented with the "goto cleanup;" due to
> lines 2000-2001 not being indented enough, but
> line 2003 clearly isn't guarded by the
> "if (!is_char_type" conditional.
>
> In gcc/function.c:locate_and_pad_parm:
>
> 4118 locate->slot_offset.constant = -initial_offset_ptr->constant;
> 4119 if (initial_offset_ptr->var)
> 4120 locate->slot_offset.var = size_binop (MINUS_EXPR, ssize_int (0),
> 4121 initial_offset_ptr->var);
> 4122
> 4123 {
> 4124 tree s2 = sizetree;
> 4125 if (where_pad != none
> 4126 && (!tree_fits_uhwi_p (sizetree)
> 4127 || (tree_to_uhwi (sizetree) * BITS_PER_UNIT) % round_boundary))
> 4128 s2 = round_up (s2, round_boundary / BITS_PER_UNIT);
> 4129 SUB_PARM_SIZE (locate->slot_offset, s2);
> 4130 }
> ^ this block is not guarded by the
> "if (initial_offset_ptr->var)"
> and the whitespace line (4122) is likely to make a
> human reader of the code read it that way also.
>
> In each case, a blank line separated the guarded code from followup code
> that wasn't guarded, and to my eyes, the blank line makes the meaning of
> the badly-indented code sufficiently clear that it seems unjustified to
> issue a -Wmisleading-indentation warning.
This makes sense to me.
Though I've been thinking about proposing a simpler and more relaxed heuristic:
if (next_stmt_exploc.line - body_exploc.line > 1)
return false;
That is, don't warn if there are any lines between the (start of the)
body statement and the next statement.
This would catch the presence of blank lines as well as code like:
if (foo)
bar (an_argument_1,
an_argument_2);
baz ();
and
if (foo)
bar ();
/* Some comment. */
baz ();
Though I am not confident that we should not warn in such cases. At
this point whether some code is misleadingly indented or not becomes
pretty subjective (so it may be better to not warn?)
>
> The attached patch suppresses the warning for such a case.
>
> 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): Don't warn if the
> guarded and unguarded code are separated by one or more entirely
> blank lines.
>
> gcc/testsuite/ChangeLog:
> * c-c++-common/Wmisleading-indentation.c (fn_40): New function.
> ---
> gcc/c-family/c-indentation.c | 58 ++++++++++++++++++++++
> .../c-c++-common/Wmisleading-indentation.c | 32 ++++++++++++
> 2 files changed, 90 insertions(+)
>
> diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
> index 5b119f7..d9d4380 100644
> --- a/gcc/c-family/c-indentation.c
> +++ b/gcc/c-family/c-indentation.c
> @@ -77,6 +77,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++)
Shouldn't this loop begin at start_line + 1?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall
2015-10-29 16:32 ` [PATCH 4/4] Add -Wmisleading-indentation to -Wall David Malcolm
@ 2015-10-29 17:42 ` Jeff Law
2015-10-30 9:16 ` Richard Biener
` (2 more replies)
0 siblings, 3 replies; 44+ messages in thread
From: Jeff Law @ 2015-10-29 17:42 UTC (permalink / raw)
To: David Malcolm, gcc-patches
On 10/29/2015 10:49 AM, David Malcolm wrote:
> Our documentation describes -Wall as enabling "all the warnings about
> constructions that some users consider questionable, and that are easy to avoid
> (or modify to prevent the warning), even in conjunction with macros."
>
> I believe that -Wmisleading-indentation meets these criteria, and is
> likely to be of benefit to users who may not read release notes; it
> warns for indentation that's misleading, but not for indentation
> that's merely bad: the former are places where a user will likely
> want to fix the code.
>
> The fix is usually easy and obvious: fix the misleadingly-indented
> code. If that isn't an option for some reason, pragmas can be used to
> turn off the warning for a particular fragment of code:
>
> #pragma GCC diagnostic push
> #pragma GCC diagnostic ignored "-Wmisleading-indentation"
> if (flag)
> x = 3;
> y = 2;
> #pragma GCC diagnostic pop
>
> -Wmisleading-indentation has been tested with a variety of indentation
> styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c)
> and on a variety of real-world projects. For example, in:
> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html
> Patrick reports:
> "Tested by building the linux, git, vim, sqlite and gdb-binutils sources
> with -Wmisleading-indentation."
>
> With the tweak earlier in this kit I believe we now have a good
> enough signal:noise ratio for this warning to be widely used; hence this
> patch adds the warning to -Wall.
>
> Bootstrapped®rtested with x86_64-pc-linux-gnu.
>
> OK for trunk?
>
> gcc/c-family/ChangeLog:
> * c.opt (Wmisleading-indentation): Add to -Wall for C and C++.
>
> gcc/ChangeLog:
> * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the
> list.
> (-Wmisleading-indentation): Update documentation to reflect
> being enabled by -Wall in C/C++.
I'm sure we'll get some grief for this :-)
Approved once we're clean in GCC. I'm going to explicitly say that
we'll have to watch for fallout, particularly as we start getting
feedback from Debian & Fedora mass-rebuilds as we approach release time.
If the fallout is too bad, we'll have to reconsider.
I'll pre-approve patches which fix anything caught by this option in GCC
as long as the fix just adjusts whitespace :-)
jeff
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines
2015-10-29 17:38 ` Patrick Palka
@ 2015-10-29 17:44 ` Patrick Palka
2015-10-29 17:50 ` Jeff Law
2015-10-29 17:56 ` Mike Stump
2015-10-29 18:13 ` David Malcolm
1 sibling, 2 replies; 44+ messages in thread
From: Patrick Palka @ 2015-10-29 17:44 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches
On Thu, Oct 29, 2015 at 1:35 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Thu, Oct 29, 2015 at 12:49 PM, David Malcolm <dmalcolm@redhat.com> wrote:
>> Attempting to bootstrap gcc with -Wmisleading-indentation enabled I ran
>> into a few failures where the indentation, although bad, was arguably
>> not misleading.
>>
>> In regrename.c:scan_rtx_address:
>>
>> 1308 case PRE_MODIFY:
>> 1309 /* If the target doesn't claim to handle autoinc, this must be
>> 1310 something special, like a stack push. Kill this chain. */
>> 1311 if (!AUTO_INC_DEC)
>> 1312 action = mark_all_read;
>> 1313
>> 1314 break;
>> ^ this is indented at the same level as the "action =" code,
>> but clearly isn't guarded by the if () at line 1311.
>>
>> In gcc/fortran/io.c:gfc_match_open:
>>
>> 1997 {
>> 1998 static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", NULL };
>> 1999
>> 2000 if (!is_char_type ("DELIM", open->delim))
>> 2001 goto cleanup;
>> 2002
>> 2003 if (!compare_to_allowed_values ("DELIM", delim, NULL, NULL,
>> 2004 open->delim->value.character.string,
>> 2005 "OPEN", warn))
>> ^ this is indented with the "goto cleanup;" due to
>> lines 2000-2001 not being indented enough, but
>> line 2003 clearly isn't guarded by the
>> "if (!is_char_type" conditional.
>>
>> In gcc/function.c:locate_and_pad_parm:
>>
>> 4118 locate->slot_offset.constant = -initial_offset_ptr->constant;
>> 4119 if (initial_offset_ptr->var)
>> 4120 locate->slot_offset.var = size_binop (MINUS_EXPR, ssize_int (0),
>> 4121 initial_offset_ptr->var);
>> 4122
>> 4123 {
>> 4124 tree s2 = sizetree;
>> 4125 if (where_pad != none
>> 4126 && (!tree_fits_uhwi_p (sizetree)
>> 4127 || (tree_to_uhwi (sizetree) * BITS_PER_UNIT) % round_boundary))
>> 4128 s2 = round_up (s2, round_boundary / BITS_PER_UNIT);
>> 4129 SUB_PARM_SIZE (locate->slot_offset, s2);
>> 4130 }
>> ^ this block is not guarded by the
>> "if (initial_offset_ptr->var)"
>> and the whitespace line (4122) is likely to make a
>> human reader of the code read it that way also.
>>
>> In each case, a blank line separated the guarded code from followup code
>> that wasn't guarded, and to my eyes, the blank line makes the meaning of
>> the badly-indented code sufficiently clear that it seems unjustified to
>> issue a -Wmisleading-indentation warning.
>
> This makes sense to me.
>
> Though I've been thinking about proposing a simpler and more relaxed heuristic:
>
> if (next_stmt_exploc.line - body_exploc.line > 1)
> return false;
>
> That is, don't warn if there are any lines between the (start of the)
> body statement and the next statement.
>
> This would catch the presence of blank lines as well as code like:
>
> if (foo)
> bar (an_argument_1,
> an_argument_2);
> baz ();
>
> and
>
> if (foo)
> bar ();
> /* Some comment. */
> baz ();
>
> Though I am not confident that we should not warn in such cases. At
> this point whether some code is misleadingly indented or not becomes
> pretty subjective (so it may be better to not warn?)
However we should definitely not warn on
if (foo)
bar ();
{
baz ();
}
Since that is valid GNU-style code :)
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines
2015-10-29 17:44 ` Patrick Palka
@ 2015-10-29 17:50 ` Jeff Law
2015-10-29 17:56 ` Mike Stump
1 sibling, 0 replies; 44+ messages in thread
From: Jeff Law @ 2015-10-29 17:50 UTC (permalink / raw)
To: Patrick Palka, David Malcolm; +Cc: GCC Patches
On 10/29/2015 11:42 AM, Patrick Palka wrote:
> On Thu, Oct 29, 2015 at 1:35 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> On Thu, Oct 29, 2015 at 12:49 PM, David Malcolm <dmalcolm@redhat.com> wrote:
>>> Attempting to bootstrap gcc with -Wmisleading-indentation enabled I ran
>>> into a few failures where the indentation, although bad, was arguably
>>> not misleading.
>>>
>>> In regrename.c:scan_rtx_address:
>>>
>>> 1308 case PRE_MODIFY:
>>> 1309 /* If the target doesn't claim to handle autoinc, this must be
>>> 1310 something special, like a stack push. Kill this chain. */
>>> 1311 if (!AUTO_INC_DEC)
>>> 1312 action = mark_all_read;
>>> 1313
>>> 1314 break;
>>> ^ this is indented at the same level as the "action =" code,
>>> but clearly isn't guarded by the if () at line 1311.
>>>
>>> In gcc/fortran/io.c:gfc_match_open:
>>>
>>> 1997 {
>>> 1998 static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", NULL };
>>> 1999
>>> 2000 if (!is_char_type ("DELIM", open->delim))
>>> 2001 goto cleanup;
>>> 2002
>>> 2003 if (!compare_to_allowed_values ("DELIM", delim, NULL, NULL,
>>> 2004 open->delim->value.character.string,
>>> 2005 "OPEN", warn))
>>> ^ this is indented with the "goto cleanup;" due to
>>> lines 2000-2001 not being indented enough, but
>>> line 2003 clearly isn't guarded by the
>>> "if (!is_char_type" conditional.
>>>
>>> In gcc/function.c:locate_and_pad_parm:
>>>
>>> 4118 locate->slot_offset.constant = -initial_offset_ptr->constant;
>>> 4119 if (initial_offset_ptr->var)
>>> 4120 locate->slot_offset.var = size_binop (MINUS_EXPR, ssize_int (0),
>>> 4121 initial_offset_ptr->var);
>>> 4122
>>> 4123 {
>>> 4124 tree s2 = sizetree;
>>> 4125 if (where_pad != none
>>> 4126 && (!tree_fits_uhwi_p (sizetree)
>>> 4127 || (tree_to_uhwi (sizetree) * BITS_PER_UNIT) % round_boundary))
>>> 4128 s2 = round_up (s2, round_boundary / BITS_PER_UNIT);
>>> 4129 SUB_PARM_SIZE (locate->slot_offset, s2);
>>> 4130 }
>>> ^ this block is not guarded by the
>>> "if (initial_offset_ptr->var)"
>>> and the whitespace line (4122) is likely to make a
>>> human reader of the code read it that way also.
>>>
>>> In each case, a blank line separated the guarded code from followup code
>>> that wasn't guarded, and to my eyes, the blank line makes the meaning of
>>> the badly-indented code sufficiently clear that it seems unjustified to
>>> issue a -Wmisleading-indentation warning.
>>
>> This makes sense to me.
>>
>> Though I've been thinking about proposing a simpler and more relaxed heuristic:
>>
>> if (next_stmt_exploc.line - body_exploc.line > 1)
>> return false;
>>
>> That is, don't warn if there are any lines between the (start of the)
>> body statement and the next statement.
>>
>> This would catch the presence of blank lines as well as code like:
>>
>> if (foo)
>> bar (an_argument_1,
>> an_argument_2);
>> baz ();
>>
>> and
>>
>> if (foo)
>> bar ();
>> /* Some comment. */
>> baz ();
>>
>> Though I am not confident that we should not warn in such cases. At
>> this point whether some code is misleadingly indented or not becomes
>> pretty subjective (so it may be better to not warn?)
>
> However we should definitely not warn on
>
> if (foo)
> bar ();
>
> {
> baz ();
> }
>
> Since that is valid GNU-style code :)
Wouldn't GNU style need the curleys lined up with the IF? and the call
to baz() indented two spaces relative to the curleys?
That's the way I've always done things when I've wanted to introduce a
binding scope like that.
jeff
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines
2015-10-29 17:56 ` Mike Stump
@ 2015-10-29 17:56 ` Patrick Palka
2015-10-29 18:00 ` Mike Stump
0 siblings, 1 reply; 44+ messages in thread
From: Patrick Palka @ 2015-10-29 17:56 UTC (permalink / raw)
To: Mike Stump; +Cc: David Malcolm, GCC Patches
On Thu, Oct 29, 2015 at 1:50 PM, Mike Stump <mikestump@comcast.net> wrote:
> On Oct 29, 2015, at 10:42 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> However we should definitely not warn on
>>
>> if (foo)
>> bar ();
>>
>> {
>> baz ();
>> }
>>
>> Since that is valid GNU-style code :)
>
> I’ll put it differently; no, that formatting is wrong.
>
Ah, okay. That simplifies things then. (I've seen that style used in
GCC and in GDB so I assumed it was valid GNU style since I couldn't
find a section in the style guide that explicitly stated how to indent
there.)
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines
2015-10-29 17:44 ` Patrick Palka
2015-10-29 17:50 ` Jeff Law
@ 2015-10-29 17:56 ` Mike Stump
2015-10-29 17:56 ` Patrick Palka
1 sibling, 1 reply; 44+ messages in thread
From: Mike Stump @ 2015-10-29 17:56 UTC (permalink / raw)
To: Patrick Palka; +Cc: David Malcolm, GCC Patches
On Oct 29, 2015, at 10:42 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> However we should definitely not warn on
>
> if (foo)
> bar ();
>
> {
> baz ();
> }
>
> Since that is valid GNU-style code :)
I’ll put it differently; no, that formatting is wrong.
^ permalink raw reply [flat|nested] 44+ messages in thread
* AW: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines
2015-10-29 16:32 ` [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines David Malcolm
2015-10-29 17:33 ` Jeff Law
2015-10-29 17:38 ` Patrick Palka
@ 2015-10-29 17:59 ` bernds_cb1
2 siblings, 0 replies; 44+ messages in thread
From: bernds_cb1 @ 2015-10-29 17:59 UTC (permalink / raw)
To: David Malcolm, gcc-patches; +Cc: David Malcolm
-----Original-Nachricht-----
Betreff: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines
Datum: 2015-10-29T17:49:38+0100
Von: "David Malcolm" <dmalcolm@redhat.com>
> In each case, a blank line separated the guarded code from followup code
> that wasn't guarded, and to my eyes, the blank line makes the meaning of
> the badly-indented code sufficiently clear that it seems unjustified to
> issue a -Wmisleading-indentation warning.
I think you could (barely) make a case for the last one being ok, but I disagree fairly strongly about the other two cases - they are clearly misindented, so why wouldn't we warn?
Bernd
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines
2015-10-29 17:56 ` Patrick Palka
@ 2015-10-29 18:00 ` Mike Stump
0 siblings, 0 replies; 44+ messages in thread
From: Mike Stump @ 2015-10-29 18:00 UTC (permalink / raw)
To: Patrick Palka; +Cc: David Malcolm, GCC Patches
On Oct 29, 2015, at 10:56 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Thu, Oct 29, 2015 at 1:50 PM, Mike Stump <mikestump@comcast.net> wrote:
>> On Oct 29, 2015, at 10:42 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>>> However we should definitely not warn on
>>>
>>> if (foo)
>>> bar ();
>>>
>>> {
>>> baz ();
>>> }
>>>
>>> Since that is valid GNU-style code :)
>>
>> I’ll put it differently; no, that formatting is wrong.
>>
>
> Ah, okay. That simplifies things then. (I've seen that style used in
> GCC and in GDB
It doses not appear in gcc or gdb.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines
2015-10-29 17:38 ` Patrick Palka
2015-10-29 17:44 ` Patrick Palka
@ 2015-10-29 18:13 ` David Malcolm
1 sibling, 0 replies; 44+ messages in thread
From: David Malcolm @ 2015-10-29 18:13 UTC (permalink / raw)
To: Patrick Palka; +Cc: GCC Patches
On Thu, 2015-10-29 at 13:35 -0400, Patrick Palka wrote:
> On Thu, Oct 29, 2015 at 12:49 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> > Attempting to bootstrap gcc with -Wmisleading-indentation enabled I ran
> > into a few failures where the indentation, although bad, was arguably
> > not misleading.
> >
> > In regrename.c:scan_rtx_address:
> >
> > 1308 case PRE_MODIFY:
> > 1309 /* If the target doesn't claim to handle autoinc, this must be
> > 1310 something special, like a stack push. Kill this chain. */
> > 1311 if (!AUTO_INC_DEC)
> > 1312 action = mark_all_read;
> > 1313
> > 1314 break;
> > ^ this is indented at the same level as the "action =" code,
> > but clearly isn't guarded by the if () at line 1311.
> >
> > In gcc/fortran/io.c:gfc_match_open:
> >
> > 1997 {
> > 1998 static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", NULL };
> > 1999
> > 2000 if (!is_char_type ("DELIM", open->delim))
> > 2001 goto cleanup;
> > 2002
> > 2003 if (!compare_to_allowed_values ("DELIM", delim, NULL, NULL,
> > 2004 open->delim->value.character.string,
> > 2005 "OPEN", warn))
> > ^ this is indented with the "goto cleanup;" due to
> > lines 2000-2001 not being indented enough, but
> > line 2003 clearly isn't guarded by the
> > "if (!is_char_type" conditional.
> >
> > In gcc/function.c:locate_and_pad_parm:
> >
> > 4118 locate->slot_offset.constant = -initial_offset_ptr->constant;
> > 4119 if (initial_offset_ptr->var)
> > 4120 locate->slot_offset.var = size_binop (MINUS_EXPR, ssize_int (0),
> > 4121 initial_offset_ptr->var);
> > 4122
> > 4123 {
> > 4124 tree s2 = sizetree;
> > 4125 if (where_pad != none
> > 4126 && (!tree_fits_uhwi_p (sizetree)
> > 4127 || (tree_to_uhwi (sizetree) * BITS_PER_UNIT) % round_boundary))
> > 4128 s2 = round_up (s2, round_boundary / BITS_PER_UNIT);
> > 4129 SUB_PARM_SIZE (locate->slot_offset, s2);
> > 4130 }
> > ^ this block is not guarded by the
> > "if (initial_offset_ptr->var)"
> > and the whitespace line (4122) is likely to make a
> > human reader of the code read it that way also.
> >
> > In each case, a blank line separated the guarded code from followup code
> > that wasn't guarded, and to my eyes, the blank line makes the meaning of
> > the badly-indented code sufficiently clear that it seems unjustified to
> > issue a -Wmisleading-indentation warning.
>
> This makes sense to me.
>
> Though I've been thinking about proposing a simpler and more relaxed heuristic:
>
> if (next_stmt_exploc.line - body_exploc.line > 1)
> return false;
>
> That is, don't warn if there are any lines between the (start of the)
> body statement and the next statement.
>
> This would catch the presence of blank lines as well as code like:
>
> if (foo)
> bar (an_argument_1,
> an_argument_2);
> baz ();
If I'm following you, I think I disagree: my (subjective) opinion is
that we ought to warn for such a case. I ran into an example of this on
the linux kernel:
https://bugzilla.kernel.org/show_bug.cgi?id=98261
In linux-4.0.3/drivers/scsi/bfa/bfa_ioc.c: bfa_cb_sfp_state_query:
3673 if (sfp->state_query_cbfn)
3674 sfp->state_query_cbfn(sfp->state_query_cbarg,
3675 sfp->status);
3676 sfp->portspeed = BFA_PORT_SPEED_UNKNOWN;
Line 3676 is indented as if guarded by line 3673's "if", when it's not.
I would want us to emit a warning about this, despite the presence of
line 3675 (body_exploc would be on line 3674, next_stmt_exploc on line
3676).
> and
>
> if (foo)
> bar ();
> /* Some comment. */
> baz ();
>
> Though I am not confident that we should not warn in such cases. At
> this point whether some code is misleadingly indented or not becomes
> pretty subjective (so it may be better to not warn?)
My (subjective) opinion is that we also ought to warn for this case.
Clearly, there's something of an "eye of the beholder" effect here. My
thinking with the blank lines approach is that if a human reader is
quickly scanning over the code, an entirely blank line is much more
noticeable than code or comments: it breaks up the apparent "structure"
of the code more. I don't have hard data to back this up though.
(My feelings on indentation and the "look" of code may be being
influenced by my Python background - I worked on Python before working
on GCC)
> > The attached patch suppresses the warning for such a case.
> >
> > 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): Don't warn if the
> > guarded and unguarded code are separated by one or more entirely
> > blank lines.
> >
> > gcc/testsuite/ChangeLog:
> > * c-c++-common/Wmisleading-indentation.c (fn_40): New function.
> > ---
> > gcc/c-family/c-indentation.c | 58 ++++++++++++++++++++++
> > .../c-c++-common/Wmisleading-indentation.c | 32 ++++++++++++
> > 2 files changed, 90 insertions(+)
> >
> > diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
> > index 5b119f7..d9d4380 100644
> > --- a/gcc/c-family/c-indentation.c
> > +++ b/gcc/c-family/c-indentation.c
> > @@ -77,6 +77,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++)
>
> Shouldn't this loop begin at start_line + 1?
Good catch; thanks.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/4] -Wmisleading-indentation
2015-10-29 16:32 [PATCH 0/4] -Wmisleading-indentation David Malcolm
` (3 preceding siblings ...)
2015-10-29 16:32 ` [PATCH 2/4] Fix misleading indentation in tree-ssa-loop-unswitch.c David Malcolm
@ 2015-10-30 5:42 ` Andi Kleen
2015-10-30 5:59 ` Jeff Law
2015-10-30 15:42 ` Mike Stump
4 siblings, 2 replies; 44+ messages in thread
From: Andi Kleen @ 2015-10-30 5:42 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches
David Malcolm <dmalcolm@redhat.com> writes:
>
> * adds -Wmisleading-indentation to -Wall
I have doubts this is a good idea. I'm sure this will break
a bazillion packages which (misguidedly) ship with -Wall -Werror.
Would be better to leave the user the choice, given that such a thing
was historically never checked before and not include it in -Wall.
-Andi
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/4] -Wmisleading-indentation
2015-10-30 5:42 ` [PATCH 0/4] -Wmisleading-indentation Andi Kleen
@ 2015-10-30 5:59 ` Jeff Law
2015-10-30 15:42 ` Mike Stump
1 sibling, 0 replies; 44+ messages in thread
From: Jeff Law @ 2015-10-30 5:59 UTC (permalink / raw)
To: Andi Kleen, David Malcolm; +Cc: gcc-patches
On 10/29/2015 11:39 PM, Andi Kleen wrote:
> David Malcolm <dmalcolm@redhat.com> writes:
>>
>> * adds -Wmisleading-indentation to -Wall
>
> I have doubts this is a good idea. I'm sure this will break
> a bazillion packages which (misguidedly) ship with -Wall -Werror.
>
> Would be better to leave the user the choice, given that such a thing
> was historically never checked before and not include it in -Wall.
The same could be said for ever adding stuff to -Wall. Hell, I've taken
arrows for changes in -Wall for decades. That doesn't mean we should
lock down -Wall for those misguided projects.
As I stated in my approval message, we'll have to watch for real world
fallout and if it's bad, reevaluate.
jeff
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall
2015-10-29 17:42 ` Jeff Law
@ 2015-10-30 9:16 ` Richard Biener
2015-11-01 22:06 ` Patrick Palka
2015-12-10 15:25 ` [committed 1/5] Fix -Wmisleading-indentation warning in function.c David Malcolm
2015-12-10 18:25 ` [PATCH 4/4] Add -Wmisleading-indentation to -Wall David Malcolm
2 siblings, 1 reply; 44+ messages in thread
From: Richard Biener @ 2015-10-30 9:16 UTC (permalink / raw)
To: Jeff Law; +Cc: David Malcolm, GCC Patches
On Thu, Oct 29, 2015 at 6:38 PM, Jeff Law <law@redhat.com> wrote:
> On 10/29/2015 10:49 AM, David Malcolm wrote:
>>
>> Our documentation describes -Wall as enabling "all the warnings about
>> constructions that some users consider questionable, and that are easy to
>> avoid
>> (or modify to prevent the warning), even in conjunction with macros."
>>
>> I believe that -Wmisleading-indentation meets these criteria, and is
>> likely to be of benefit to users who may not read release notes; it
>> warns for indentation that's misleading, but not for indentation
>> that's merely bad: the former are places where a user will likely
>> want to fix the code.
>>
>> The fix is usually easy and obvious: fix the misleadingly-indented
>> code. If that isn't an option for some reason, pragmas can be used to
>> turn off the warning for a particular fragment of code:
>>
>> #pragma GCC diagnostic push
>> #pragma GCC diagnostic ignored "-Wmisleading-indentation"
>> if (flag)
>> x = 3;
>> y = 2;
>> #pragma GCC diagnostic pop
>>
>> -Wmisleading-indentation has been tested with a variety of indentation
>> styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c)
>> and on a variety of real-world projects. For example, in:
>> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html
>> Patrick reports:
>> "Tested by building the linux, git, vim, sqlite and gdb-binutils sources
>> with -Wmisleading-indentation."
>>
>> With the tweak earlier in this kit I believe we now have a good
>> enough signal:noise ratio for this warning to be widely used; hence this
>> patch adds the warning to -Wall.
>>
>> Bootstrapped®rtested with x86_64-pc-linux-gnu.
>>
>> OK for trunk?
>>
>> gcc/c-family/ChangeLog:
>> * c.opt (Wmisleading-indentation): Add to -Wall for C and C++.
>>
>> gcc/ChangeLog:
>> * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the
>> list.
>> (-Wmisleading-indentation): Update documentation to reflect
>> being enabled by -Wall in C/C++.
>
> I'm sure we'll get some grief for this :-)
>
> Approved once we're clean in GCC. I'm going to explicitly say that we'll
> have to watch for fallout, particularly as we start getting feedback from
> Debian & Fedora mass-rebuilds as we approach release time. If the fallout
> is too bad, we'll have to reconsider.
>
> I'll pre-approve patches which fix anything caught by this option in GCC as
> long as the fix just adjusts whitespace :-)
Please at least check also binutils and gdb and other packages that use -Werror
(well, just rebuild Fedora world).
I'd say this shouldn't be in -Wall ... (and I suppose I'll happily
patch it out of SUSE
GCC ...). Maybe put it into -Wextra?
Richard.
> jeff
>
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 0/4] -Wmisleading-indentation
2015-10-30 5:42 ` [PATCH 0/4] -Wmisleading-indentation Andi Kleen
2015-10-30 5:59 ` Jeff Law
@ 2015-10-30 15:42 ` Mike Stump
1 sibling, 0 replies; 44+ messages in thread
From: Mike Stump @ 2015-10-30 15:42 UTC (permalink / raw)
To: Andi Kleen; +Cc: David Malcolm, gcc-patches
On Oct 29, 2015, at 10:39 PM, Andi Kleen <andi@firstfloor.org> wrote:
> David Malcolm <dmalcolm@redhat.com> writes:
>>
>> * adds -Wmisleading-indentation to -Wall
>
> I have doubts this is a good idea. I'm sure this will break
> a bazillion packages which (misguidedly) ship with -Wall -Werror.
>
> Would be better to leave the user the choice, given that such a thing
> was historically never checked before and not include it in -Wall.
So, my experience is that folks that run large scale builds have ways of adding a flag like -Wno-misleading-indentation to all builds or otherwise turning off -Werror for exactly this reason. An OS build from source does make for a nice smoke test for the feature. These people also usually fix(ate) the compiler and don’t update it, for much the same reason. The plan would be for gcc to update, packages to update, the OS or package integrator then pull in the updated packages and the updated compiler and then release. As this time, things would then be ok. We should turn it on now (for trunk only), and send in fixes and bug reports to all packages that don’t build. Those packages then have time to update. When we go for release, maybe the RM decides that we will give people 1 more year to fix their code and leave it out of -Wall for 1 more year, if there are too many packages dragging their feet. I think that’s a fine use of an RM. We can record in a gcc PR to disable for Wall the packages that break, and then consider disabling for Wall if the package list remains too long. If the list is short by the time we go to release, then the RM closes as not to be fixed. This is why I favor putting the change in Wall now (but subject to reversion on all release branches at RM discretion).
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall
2015-10-30 9:16 ` Richard Biener
@ 2015-11-01 22:06 ` Patrick Palka
2015-11-02 16:21 ` David Malcolm
0 siblings, 1 reply; 44+ messages in thread
From: Patrick Palka @ 2015-11-01 22:06 UTC (permalink / raw)
To: Richard Biener; +Cc: Jeff Law, David Malcolm, GCC Patches
On Fri, Oct 30, 2015 at 5:03 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Oct 29, 2015 at 6:38 PM, Jeff Law <law@redhat.com> wrote:
>> On 10/29/2015 10:49 AM, David Malcolm wrote:
>>>
>>> Our documentation describes -Wall as enabling "all the warnings about
>>> constructions that some users consider questionable, and that are easy to
>>> avoid
>>> (or modify to prevent the warning), even in conjunction with macros."
>>>
>>> I believe that -Wmisleading-indentation meets these criteria, and is
>>> likely to be of benefit to users who may not read release notes; it
>>> warns for indentation that's misleading, but not for indentation
>>> that's merely bad: the former are places where a user will likely
>>> want to fix the code.
>>>
>>> The fix is usually easy and obvious: fix the misleadingly-indented
>>> code. If that isn't an option for some reason, pragmas can be used to
>>> turn off the warning for a particular fragment of code:
>>>
>>> #pragma GCC diagnostic push
>>> #pragma GCC diagnostic ignored "-Wmisleading-indentation"
>>> if (flag)
>>> x = 3;
>>> y = 2;
>>> #pragma GCC diagnostic pop
>>>
>>> -Wmisleading-indentation has been tested with a variety of indentation
>>> styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c)
>>> and on a variety of real-world projects. For example, in:
>>> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html
>>> Patrick reports:
>>> "Tested by building the linux, git, vim, sqlite and gdb-binutils sources
>>> with -Wmisleading-indentation."
>>>
>>> With the tweak earlier in this kit I believe we now have a good
>>> enough signal:noise ratio for this warning to be widely used; hence this
>>> patch adds the warning to -Wall.
>>>
>>> Bootstrapped®rtested with x86_64-pc-linux-gnu.
>>>
>>> OK for trunk?
>>>
>>> gcc/c-family/ChangeLog:
>>> * c.opt (Wmisleading-indentation): Add to -Wall for C and C++.
>>>
>>> gcc/ChangeLog:
>>> * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the
>>> list.
>>> (-Wmisleading-indentation): Update documentation to reflect
>>> being enabled by -Wall in C/C++.
>>
>> I'm sure we'll get some grief for this :-)
>>
>> Approved once we're clean in GCC. I'm going to explicitly say that we'll
>> have to watch for fallout, particularly as we start getting feedback from
>> Debian & Fedora mass-rebuilds as we approach release time. If the fallout
>> is too bad, we'll have to reconsider.
>>
>> I'll pre-approve patches which fix anything caught by this option in GCC as
>> long as the fix just adjusts whitespace :-)
>
> Please at least check also binutils and gdb and other packages that use -Werror
> (well, just rebuild Fedora world).
FYI binutils, gdb and glibc, from git, all fail to build due to
-Wmisleading-indentation warnings/errors. (None of the warnings are
bogus (IMO), though I don't think any of the warnings expose a real
bug either.)
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall
2015-11-01 22:06 ` Patrick Palka
@ 2015-11-02 16:21 ` David Malcolm
2015-11-02 17:11 ` David Malcolm
2015-11-02 18:39 ` Patrick Palka
0 siblings, 2 replies; 44+ messages in thread
From: David Malcolm @ 2015-11-02 16:21 UTC (permalink / raw)
To: Patrick Palka; +Cc: Richard Biener, Jeff Law, GCC Patches
On Sun, 2015-11-01 at 17:06 -0500, Patrick Palka wrote:
> On Fri, Oct 30, 2015 at 5:03 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
> > On Thu, Oct 29, 2015 at 6:38 PM, Jeff Law <law@redhat.com> wrote:
> >> On 10/29/2015 10:49 AM, David Malcolm wrote:
> >>>
> >>> Our documentation describes -Wall as enabling "all the warnings about
> >>> constructions that some users consider questionable, and that are easy to
> >>> avoid
> >>> (or modify to prevent the warning), even in conjunction with macros."
> >>>
> >>> I believe that -Wmisleading-indentation meets these criteria, and is
> >>> likely to be of benefit to users who may not read release notes; it
> >>> warns for indentation that's misleading, but not for indentation
> >>> that's merely bad: the former are places where a user will likely
> >>> want to fix the code.
> >>>
> >>> The fix is usually easy and obvious: fix the misleadingly-indented
> >>> code. If that isn't an option for some reason, pragmas can be used to
> >>> turn off the warning for a particular fragment of code:
> >>>
> >>> #pragma GCC diagnostic push
> >>> #pragma GCC diagnostic ignored "-Wmisleading-indentation"
> >>> if (flag)
> >>> x = 3;
> >>> y = 2;
> >>> #pragma GCC diagnostic pop
> >>>
> >>> -Wmisleading-indentation has been tested with a variety of indentation
> >>> styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c)
> >>> and on a variety of real-world projects. For example, in:
> >>> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html
> >>> Patrick reports:
> >>> "Tested by building the linux, git, vim, sqlite and gdb-binutils sources
> >>> with -Wmisleading-indentation."
> >>>
> >>> With the tweak earlier in this kit I believe we now have a good
> >>> enough signal:noise ratio for this warning to be widely used; hence this
> >>> patch adds the warning to -Wall.
> >>>
> >>> Bootstrapped®rtested with x86_64-pc-linux-gnu.
> >>>
> >>> OK for trunk?
> >>>
> >>> gcc/c-family/ChangeLog:
> >>> * c.opt (Wmisleading-indentation): Add to -Wall for C and C++.
> >>>
> >>> gcc/ChangeLog:
> >>> * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the
> >>> list.
> >>> (-Wmisleading-indentation): Update documentation to reflect
> >>> being enabled by -Wall in C/C++.
> >>
> >> I'm sure we'll get some grief for this :-)
> >>
> >> Approved once we're clean in GCC. I'm going to explicitly say that we'll
> >> have to watch for fallout, particularly as we start getting feedback from
> >> Debian & Fedora mass-rebuilds as we approach release time. If the fallout
> >> is too bad, we'll have to reconsider.
> >>
> >> I'll pre-approve patches which fix anything caught by this option in GCC as
> >> long as the fix just adjusts whitespace :-)
> >
> > Please at least check also binutils and gdb and other packages that use -Werror
> > (well, just rebuild Fedora world).
>
> FYI binutils, gdb and glibc, from git, all fail to build due to
> -Wmisleading-indentation warnings/errors. (None of the warnings are
> bogus (IMO), though I don't think any of the warnings expose a real
> bug either.)
Bother. Do you happen to have the logs handy? (or could you upload the
somewhere?)
I tried building binutils+gdb 854eb72b00ba46d65ce36dc3432f01e223ce44cb
with gcc6+this kit (on x86_64) but I get:
In file included from ../../src/bfd/archive.c:143:0:
../../src/bfd/../include/bfdlink.h:452:38: error: field âcompress_debugâ
has incomplete type
enum compressed_debug_section_type compress_debug;
^
../../src/bfd/archive.c: In function âopen_nested_fileâ:
../../src/bfd/archive.c:393:12: error: âbfd {aka struct bfd}â has no
member named âlto_outputâ
n_bfd->lto_output = archive->lto_output;
^
which appears to be unrelated snafu from the binutils+gdb side.
Thanks
Dave
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall
2015-11-02 16:21 ` David Malcolm
@ 2015-11-02 17:11 ` David Malcolm
2015-11-02 18:39 ` Patrick Palka
1 sibling, 0 replies; 44+ messages in thread
From: David Malcolm @ 2015-11-02 17:11 UTC (permalink / raw)
To: Patrick Palka; +Cc: Richard Biener, Jeff Law, GCC Patches
On Mon, 2015-11-02 at 11:21 -0500, David Malcolm wrote:
> On Sun, 2015-11-01 at 17:06 -0500, Patrick Palka wrote:
> > On Fri, Oct 30, 2015 at 5:03 AM, Richard Biener
> > <richard.guenther@gmail.com> wrote:
> > > On Thu, Oct 29, 2015 at 6:38 PM, Jeff Law <law@redhat.com> wrote:
> > >> On 10/29/2015 10:49 AM, David Malcolm wrote:
> > >>>
> > >>> Our documentation describes -Wall as enabling "all the warnings about
> > >>> constructions that some users consider questionable, and that are easy to
> > >>> avoid
> > >>> (or modify to prevent the warning), even in conjunction with macros."
> > >>>
> > >>> I believe that -Wmisleading-indentation meets these criteria, and is
> > >>> likely to be of benefit to users who may not read release notes; it
> > >>> warns for indentation that's misleading, but not for indentation
> > >>> that's merely bad: the former are places where a user will likely
> > >>> want to fix the code.
> > >>>
> > >>> The fix is usually easy and obvious: fix the misleadingly-indented
> > >>> code. If that isn't an option for some reason, pragmas can be used to
> > >>> turn off the warning for a particular fragment of code:
> > >>>
> > >>> #pragma GCC diagnostic push
> > >>> #pragma GCC diagnostic ignored "-Wmisleading-indentation"
> > >>> if (flag)
> > >>> x = 3;
> > >>> y = 2;
> > >>> #pragma GCC diagnostic pop
> > >>>
> > >>> -Wmisleading-indentation has been tested with a variety of indentation
> > >>> styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c)
> > >>> and on a variety of real-world projects. For example, in:
> > >>> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html
> > >>> Patrick reports:
> > >>> "Tested by building the linux, git, vim, sqlite and gdb-binutils sources
> > >>> with -Wmisleading-indentation."
> > >>>
> > >>> With the tweak earlier in this kit I believe we now have a good
> > >>> enough signal:noise ratio for this warning to be widely used; hence this
> > >>> patch adds the warning to -Wall.
> > >>>
> > >>> Bootstrapped®rtested with x86_64-pc-linux-gnu.
> > >>>
> > >>> OK for trunk?
> > >>>
> > >>> gcc/c-family/ChangeLog:
> > >>> * c.opt (Wmisleading-indentation): Add to -Wall for C and C++.
> > >>>
> > >>> gcc/ChangeLog:
> > >>> * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the
> > >>> list.
> > >>> (-Wmisleading-indentation): Update documentation to reflect
> > >>> being enabled by -Wall in C/C++.
> > >>
> > >> I'm sure we'll get some grief for this :-)
> > >>
> > >> Approved once we're clean in GCC. I'm going to explicitly say that we'll
> > >> have to watch for fallout, particularly as we start getting feedback from
> > >> Debian & Fedora mass-rebuilds as we approach release time. If the fallout
> > >> is too bad, we'll have to reconsider.
> > >>
> > >> I'll pre-approve patches which fix anything caught by this option in GCC as
> > >> long as the fix just adjusts whitespace :-)
> > >
> > > Please at least check also binutils and gdb and other packages that use -Werror
> > > (well, just rebuild Fedora world).
> >
> > FYI binutils, gdb and glibc, from git, all fail to build due to
> > -Wmisleading-indentation warnings/errors. (None of the warnings are
> > bogus (IMO), though I don't think any of the warnings expose a real
> > bug either.)
>
> Bother. Do you happen to have the logs handy? (or could you upload the
> somewhere?)
>
> I tried building binutils+gdb 854eb72b00ba46d65ce36dc3432f01e223ce44cb
> with gcc6+this kit (on x86_64) but I get:
> In file included from ../../src/bfd/archive.c:143:0:
> ../../src/bfd/../include/bfdlink.h:452:38: error: field âcompress_debugâ
> has incomplete type
> enum compressed_debug_section_type compress_debug;
> ^
> ../../src/bfd/archive.c: In function âopen_nested_fileâ:
> ../../src/bfd/archive.c:393:12: error: âbfd {aka struct bfd}â has no
> member named âlto_outputâ
> n_bfd->lto_output = archive->lto_output;
> ^
> which appears to be unrelated snafu from the binutils+gdb side.
The only one I saw in glibc was this:
../stdlib/strtol_l.c: In function â____strtoul_l_internalâ:
../stdlib/strtol_l.c:356:9: error: statement is indented as if it were
guarded by... [-Werror=misleading-indentation]
cnt < thousands_len; })
^
../stdlib/strtol_l.c:353:9: note: ...this âforâ clause, but it is not
&& ({ for (cnt = 0; cnt < thousands_len; ++cnt)
^
where the code is question looks like this:
348 for (c = *end; c != L_('\0'); c = *++end)
349 if (((STRING_TYPE) c < L_('0') || (STRING_TYPE) c > L_('9'))
350 # ifdef USE_WIDE_CHAR
351 && (wchar_t) c != thousands
352 # else
353 && ({ for (cnt = 0; cnt < thousands_len; ++cnt)
354 if (thousands[cnt] != end[cnt])
355 break;
356 cnt < thousands_len; })
357 # endif
358 && (!ISALPHA (c)
359 || (int) (TOUPPER (c) - L_('A') + 10) >= base))
360 break;
It looks like lines 354 and 355 are poorly indented, leading to the
warning from -Wmisleading-indentation at line 356.
It could be argued that the warning is reasonable here, though I don't
like the wording of our warning here: line 356 isn't indented as if
guarded by line 353, it's more that lines 354 and 355 *aren't* indented.
I've filed PR 68187 to track this.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall
2015-11-02 16:21 ` David Malcolm
2015-11-02 17:11 ` David Malcolm
@ 2015-11-02 18:39 ` Patrick Palka
2015-11-02 19:35 ` David Malcolm
1 sibling, 1 reply; 44+ messages in thread
From: Patrick Palka @ 2015-11-02 18:39 UTC (permalink / raw)
To: David Malcolm; +Cc: Patrick Palka, Richard Biener, Jeff Law, GCC Patches
[-- Attachment #1: Type: text/plain, Size: 11775 bytes --]
On Mon, 2 Nov 2015, David Malcolm wrote:
> On Sun, 2015-11-01 at 17:06 -0500, Patrick Palka wrote:
>> On Fri, Oct 30, 2015 at 5:03 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>> On Thu, Oct 29, 2015 at 6:38 PM, Jeff Law <law@redhat.com> wrote:
>>>> On 10/29/2015 10:49 AM, David Malcolm wrote:
>>>>>
>>>>> Our documentation describes -Wall as enabling "all the warnings about
>>>>> constructions that some users consider questionable, and that are easy to
>>>>> avoid
>>>>> (or modify to prevent the warning), even in conjunction with macros."
>>>>>
>>>>> I believe that -Wmisleading-indentation meets these criteria, and is
>>>>> likely to be of benefit to users who may not read release notes; it
>>>>> warns for indentation that's misleading, but not for indentation
>>>>> that's merely bad: the former are places where a user will likely
>>>>> want to fix the code.
>>>>>
>>>>> The fix is usually easy and obvious: fix the misleadingly-indented
>>>>> code. If that isn't an option for some reason, pragmas can be used to
>>>>> turn off the warning for a particular fragment of code:
>>>>>
>>>>> #pragma GCC diagnostic push
>>>>> #pragma GCC diagnostic ignored "-Wmisleading-indentation"
>>>>> if (flag)
>>>>> x = 3;
>>>>> y = 2;
>>>>> #pragma GCC diagnostic pop
>>>>>
>>>>> -Wmisleading-indentation has been tested with a variety of indentation
>>>>> styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c)
>>>>> and on a variety of real-world projects. For example, in:
>>>>> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html
>>>>> Patrick reports:
>>>>> "Tested by building the linux, git, vim, sqlite and gdb-binutils sources
>>>>> with -Wmisleading-indentation."
>>>>>
>>>>> With the tweak earlier in this kit I believe we now have a good
>>>>> enough signal:noise ratio for this warning to be widely used; hence this
>>>>> patch adds the warning to -Wall.
>>>>>
>>>>> Bootstrapped®rtested with x86_64-pc-linux-gnu.
>>>>>
>>>>> OK for trunk?
>>>>>
>>>>> gcc/c-family/ChangeLog:
>>>>> * c.opt (Wmisleading-indentation): Add to -Wall for C and C++.
>>>>>
>>>>> gcc/ChangeLog:
>>>>> * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the
>>>>> list.
>>>>> (-Wmisleading-indentation): Update documentation to reflect
>>>>> being enabled by -Wall in C/C++.
>>>>
>>>> I'm sure we'll get some grief for this :-)
>>>>
>>>> Approved once we're clean in GCC. I'm going to explicitly say that we'll
>>>> have to watch for fallout, particularly as we start getting feedback from
>>>> Debian & Fedora mass-rebuilds as we approach release time. If the fallout
>>>> is too bad, we'll have to reconsider.
>>>>
>>>> I'll pre-approve patches which fix anything caught by this option in GCC as
>>>> long as the fix just adjusts whitespace :-)
>>>
>>> Please at least check also binutils and gdb and other packages that use -Werror
>>> (well, just rebuild Fedora world).
>>
>> FYI binutils, gdb and glibc, from git, all fail to build due to
>> -Wmisleading-indentation warnings/errors. (None of the warnings are
>> bogus (IMO), though I don't think any of the warnings expose a real
>> bug either.)
>
> Bother. Do you happen to have the logs handy? (or could you upload the
> somewhere?)
>
> I tried building binutils+gdb 854eb72b00ba46d65ce36dc3432f01e223ce44cb
> with gcc6+this kit (on x86_64) but I get:
> In file included from ../../src/bfd/archive.c:143:0:
> ../../src/bfd/../include/bfdlink.h:452:38: error: field âcompress_debugâ
> has incomplete type
> enum compressed_debug_section_type compress_debug;
> ^
> ../../src/bfd/archive.c: In function âopen_nested_fileâ:
> ../../src/bfd/archive.c:393:12: error: âbfd {aka struct bfd}â has no
> member named âlto_outputâ
> n_bfd->lto_output = archive->lto_output;
> ^
> which appears to be unrelated snafu from the binutils+gdb side.
>
> Thanks
> Dave
>
>
I don't have build logs but I have diffs that indicates where in the
code the warnings appear and how to address the warnings (roughly).
For binutils-gdb:
diff --git a/bfd/coff-i386.c b/bfd/coff-i386.c
index a9725c4..fca7887 100644
--- a/bfd/coff-i386.c
+++ b/bfd/coff-i386.c
@@ -139,7 +139,7 @@ coff_i386_reloc (bfd *abfd,
#define DOIT(x) \
x = ((x & ~howto->dst_mask) | (((x & howto->src_mask) + diff) & howto->dst_mask))
- if (diff != 0)
+ if (diff != 0)
{
reloc_howto_type *howto = reloc_entry->howto;
unsigned char *addr = (unsigned char *) data + reloc_entry->address;
diff --git a/bfd/coff-x86_64.c b/bfd/coff-x86_64.c
index 4e6420a..23ffecb 100644
--- a/bfd/coff-x86_64.c
+++ b/bfd/coff-x86_64.c
@@ -138,7 +138,7 @@ coff_amd64_reloc (bfd *abfd,
#define DOIT(x) \
x = ((x & ~howto->dst_mask) | (((x & howto->src_mask) + diff) & howto->dst_mask))
- if (diff != 0)
+ if (diff != 0)
{
reloc_howto_type *howto = reloc_entry->howto;
unsigned char *addr = (unsigned char *) data + reloc_entry->address;
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
diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index 1af477c..1bd3b12 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -1305,26 +1305,26 @@ c_type_print_base (struct type *type, struct ui_file *stream,
if (TYPE_NFIELDS (type) != 0 || TYPE_NFN_FIELDS (type) != 0)
fprintf_filtered (stream, "\n");
- for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
- {
- struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
-
- /* Dereference the typedef declaration itself. */
- gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
- target = TYPE_TARGET_TYPE (target);
-
- print_spaces_filtered (level + 4, stream);
- fprintf_filtered (stream, "typedef ");
-
- /* We want to print typedefs with substitutions
- from the template parameters or globally-known
- typedefs but not local typedefs. */
- c_print_type (target,
- TYPE_TYPEDEF_FIELD_NAME (type, i),
- stream, show - 1, level + 4,
- &semi_local_flags);
- fprintf_filtered (stream, ";\n");
- }
+ for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
+ {
+ struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
+
+ /* Dereference the typedef declaration itself. */
+ gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
+ target = TYPE_TARGET_TYPE (target);
+
+ print_spaces_filtered (level + 4, stream);
+ fprintf_filtered (stream, "typedef ");
+
+ /* We want to print typedefs with substitutions
+ from the template parameters or globally-known
+ typedefs but not local typedefs. */
+ c_print_type (target,
+ TYPE_TYPEDEF_FIELD_NAME (type, i),
+ stream, show - 1, level + 4,
+ &semi_local_flags);
+ fprintf_filtered (stream, ";\n");
+ }
}
fprintfi_filtered (level, stream, "}");
diff --git a/gdb/inflow.c b/gdb/inflow.c
index d38a43d..87988ea 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -413,7 +413,7 @@ child_terminal_ours_1 (int output_only)
if (tinfo->run_terminal != NULL || gdb_has_a_terminal () == 0)
return;
- {
+ {
#ifdef SIGTTOU
/* Ignore this signal since it will happen when we try to set the
pgrp. */
@@ -497,7 +497,7 @@ child_terminal_ours_1 (int output_only)
result = fcntl (0, F_SETFL, our_terminal_info.tflags);
result = fcntl (0, F_SETFL, our_terminal_info.tflags);
#endif
- }
+ }
}
/* Per-inferior data key. */
diff --git a/gdb/linux-record.c b/gdb/linux-record.c
index 091ac8a..18f8fbf 100644
--- a/gdb/linux-record.c
+++ b/gdb/linux-record.c
@@ -112,7 +112,7 @@ record_linux_sockaddr (struct regcache *regcache,
"memory at addr = 0x%s len = %d.\n",
phex_nz (len, tdep->size_pointer),
tdep->size_int);
- return -1;
+ return -1;
}
addrlen = (int) extract_unsigned_integer (a, tdep->size_int, byte_order);
if (addrlen <= 0 || addrlen > tdep->size_sockaddr)
@@ -150,7 +150,7 @@ record_linux_msghdr (struct regcache *regcache,
"len = %d.\n",
phex_nz (addr, tdep->size_pointer),
tdep->size_msghdr);
- return -1;
+ return -1;
}
/* msg_name msg_namelen */
@@ -188,7 +188,7 @@ record_linux_msghdr (struct regcache *regcache,
"len = %d.\n",
phex_nz (addr,tdep->size_pointer),
tdep->size_iovec);
- return -1;
+ return -1;
}
tmpaddr = (CORE_ADDR) extract_unsigned_integer (iov,
tdep->size_pointer,
@@ -983,7 +983,7 @@ Do you want to stop the program?"),
"memory at addr = 0x%s len = %d.\n",
OUTPUT_REG (tmpulongest, tdep->arg2),
tdep->size_ulong);
- return -1;
+ return -1;
}
tmpulongest = extract_unsigned_integer (a, tdep->size_ulong,
byte_order);
====
And for glibc:
diff --git a/stdio-common/vfscanf.c b/stdio-common/vfscanf.c
index 1382eb5..1963c31 100644
--- a/stdio-common/vfscanf.c
+++ b/stdio-common/vfscanf.c
@@ -1711,7 +1711,7 @@ _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
/* The last thousands character will be added back by
the char_buffer_add below. */
- --charbuf.current;
+ --charbuf.current;
#endif
}
else
diff --git a/stdlib/strtol_l.c b/stdlib/strtol_l.c
index 8f6163d..0fb9a92 100644
--- a/stdlib/strtol_l.c
+++ b/stdlib/strtol_l.c
@@ -351,8 +351,8 @@ INTERNAL (__strtol_l) (const STRING_TYPE *nptr, STRING_TYPE **endptr,
&& (wchar_t) c != thousands
# else
&& ({ for (cnt = 0; cnt < thousands_len; ++cnt)
- if (thousands[cnt] != end[cnt])
- break;
+ if (thousands[cnt] != end[cnt])
+ break;
cnt < thousands_len; })
# endif
&& (!ISALPHA (c)
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;
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall
2015-11-02 18:39 ` Patrick Palka
@ 2015-11-02 19:35 ` David Malcolm
2015-11-02 20:35 ` Marc Glisse
2015-11-02 23:41 ` Jeff Law
0 siblings, 2 replies; 44+ messages in thread
From: David Malcolm @ 2015-11-02 19:35 UTC (permalink / raw)
To: Patrick Palka; +Cc: Richard Biener, Jeff Law, GCC Patches
On Mon, 2015-11-02 at 13:39 -0500, Patrick Palka wrote:
> On Mon, 2 Nov 2015, David Malcolm wrote:
>
> > On Sun, 2015-11-01 at 17:06 -0500, Patrick Palka wrote:
> >> On Fri, Oct 30, 2015 at 5:03 AM, Richard Biener
> >> <richard.guenther@gmail.com> wrote:
> >>> On Thu, Oct 29, 2015 at 6:38 PM, Jeff Law <law@redhat.com> wrote:
> >>>> On 10/29/2015 10:49 AM, David Malcolm wrote:
> >>>>>
> >>>>> Our documentation describes -Wall as enabling "all the warnings about
> >>>>> constructions that some users consider questionable, and that are easy to
> >>>>> avoid
> >>>>> (or modify to prevent the warning), even in conjunction with macros."
> >>>>>
> >>>>> I believe that -Wmisleading-indentation meets these criteria, and is
> >>>>> likely to be of benefit to users who may not read release notes; it
> >>>>> warns for indentation that's misleading, but not for indentation
> >>>>> that's merely bad: the former are places where a user will likely
> >>>>> want to fix the code.
> >>>>>
> >>>>> The fix is usually easy and obvious: fix the misleadingly-indented
> >>>>> code. If that isn't an option for some reason, pragmas can be used to
> >>>>> turn off the warning for a particular fragment of code:
> >>>>>
> >>>>> #pragma GCC diagnostic push
> >>>>> #pragma GCC diagnostic ignored "-Wmisleading-indentation"
> >>>>> if (flag)
> >>>>> x = 3;
> >>>>> y = 2;
> >>>>> #pragma GCC diagnostic pop
> >>>>>
> >>>>> -Wmisleading-indentation has been tested with a variety of indentation
> >>>>> styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c)
> >>>>> and on a variety of real-world projects. For example, in:
> >>>>> https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html
> >>>>> Patrick reports:
> >>>>> "Tested by building the linux, git, vim, sqlite and gdb-binutils sources
> >>>>> with -Wmisleading-indentation."
> >>>>>
> >>>>> With the tweak earlier in this kit I believe we now have a good
> >>>>> enough signal:noise ratio for this warning to be widely used; hence this
> >>>>> patch adds the warning to -Wall.
> >>>>>
> >>>>> Bootstrapped®rtested with x86_64-pc-linux-gnu.
> >>>>>
> >>>>> OK for trunk?
> >>>>>
> >>>>> gcc/c-family/ChangeLog:
> >>>>> * c.opt (Wmisleading-indentation): Add to -Wall for C and C++.
> >>>>>
> >>>>> gcc/ChangeLog:
> >>>>> * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the
> >>>>> list.
> >>>>> (-Wmisleading-indentation): Update documentation to reflect
> >>>>> being enabled by -Wall in C/C++.
> >>>>
> >>>> I'm sure we'll get some grief for this :-)
> >>>>
> >>>> Approved once we're clean in GCC. I'm going to explicitly say that we'll
> >>>> have to watch for fallout, particularly as we start getting feedback from
> >>>> Debian & Fedora mass-rebuilds as we approach release time. If the fallout
> >>>> is too bad, we'll have to reconsider.
> >>>>
> >>>> I'll pre-approve patches which fix anything caught by this option in GCC as
> >>>> long as the fix just adjusts whitespace :-)
> >>>
> >>> Please at least check also binutils and gdb and other packages that use -Werror
> >>> (well, just rebuild Fedora world).
> >>
> >> FYI binutils, gdb and glibc, from git, all fail to build due to
> >> -Wmisleading-indentation warnings/errors. (None of the warnings are
> >> bogus (IMO), though I don't think any of the warnings expose a real
> >> bug either.)
> >
> > Bother. Do you happen to have the logs handy? (or could you upload the
> > somewhere?)
> >
> > I tried building binutils+gdb 854eb72b00ba46d65ce36dc3432f01e223ce44cb
> > with gcc6+this kit (on x86_64) but I get:
> > In file included from ../../src/bfd/archive.c:143:0:
> > ../../src/bfd/../include/bfdlink.h:452:38: error: field âcompress_debugâ
> > has incomplete type
> > enum compressed_debug_section_type compress_debug;
> > ^
> > ../../src/bfd/archive.c: In function âopen_nested_fileâ:
> > ../../src/bfd/archive.c:393:12: error: âbfd {aka struct bfd}â has no
> > member named âlto_outputâ
> > n_bfd->lto_output = archive->lto_output;
> > ^
> > which appears to be unrelated snafu from the binutils+gdb side.
> >
> > Thanks
> > Dave
> >
> >
>
> I don't have build logs but I have diffs that indicates where in the
> code the warnings appear and how to address the warnings (roughly).
Thanks.
> For binutils-gdb:
>
> diff --git a/bfd/coff-i386.c b/bfd/coff-i386.c
> index a9725c4..fca7887 100644
> --- a/bfd/coff-i386.c
> +++ b/bfd/coff-i386.c
> @@ -139,7 +139,7 @@ coff_i386_reloc (bfd *abfd,
> #define DOIT(x) \
> x = ((x & ~howto->dst_mask) | (((x & howto->src_mask) + diff) & howto->dst_mask))
>
> - if (diff != 0)
> + if (diff != 0)
> {
> reloc_howto_type *howto = reloc_entry->howto;
> unsigned char *addr = (unsigned char *) data + reloc_entry->address;
This one has a fully blank line, so patch [1/4] would have suppressed
it.
> diff --git a/bfd/coff-x86_64.c b/bfd/coff-x86_64.c
> index 4e6420a..23ffecb 100644
> --- a/bfd/coff-x86_64.c
> +++ b/bfd/coff-x86_64.c
> @@ -138,7 +138,7 @@ coff_amd64_reloc (bfd *abfd,
> #define DOIT(x) \
> x = ((x & ~howto->dst_mask) | (((x & howto->src_mask) + diff) & howto->dst_mask))
>
> - if (diff != 0)
> + if (diff != 0)
> {
> reloc_howto_type *howto = reloc_entry->howto;
> unsigned char *addr = (unsigned char *) data + reloc_entry->address;
Likewise.
> 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.
> diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index 1af477c..1bd3b12 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> @@ -1305,26 +1305,26 @@ c_type_print_base (struct type *type, struct ui_file *stream,
> if (TYPE_NFIELDS (type) != 0 || TYPE_NFN_FIELDS (type) != 0)
> fprintf_filtered (stream, "\n");
>
> - for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
> - {
> - struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
> -
> - /* Dereference the typedef declaration itself. */
> - gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
> - target = TYPE_TARGET_TYPE (target);
> -
> - print_spaces_filtered (level + 4, stream);
> - fprintf_filtered (stream, "typedef ");
> -
> - /* We want to print typedefs with substitutions
> - from the template parameters or globally-known
> - typedefs but not local typedefs. */
> - c_print_type (target,
> - TYPE_TYPEDEF_FIELD_NAME (type, i),
> - stream, show - 1, level + 4,
> - &semi_local_flags);
> - fprintf_filtered (stream, ";\n");
> - }
> + for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
> + {
> + struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
> +
> + /* Dereference the typedef declaration itself. */
> + gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
> + target = TYPE_TARGET_TYPE (target);
> +
> + print_spaces_filtered (level + 4, stream);
> + fprintf_filtered (stream, "typedef ");
> +
> + /* We want to print typedefs with substitutions
> + from the template parameters or globally-known
> + typedefs but not local typedefs. */
> + c_print_type (target,
> + TYPE_TYPEDEF_FIELD_NAME (type, i),
> + stream, show - 1, level + 4,
> + &semi_local_flags);
> + fprintf_filtered (stream, ";\n");
> + }
> }
Likewise, the blank line means patch [1/4] would have suppressed it.
> fprintfi_filtered (level, stream, "}");
> diff --git a/gdb/inflow.c b/gdb/inflow.c
> index d38a43d..87988ea 100644
> --- a/gdb/inflow.c
> +++ b/gdb/inflow.c
> @@ -413,7 +413,7 @@ child_terminal_ours_1 (int output_only)
> if (tinfo->run_terminal != NULL || gdb_has_a_terminal () == 0)
> return;
>
> - {
> + {
> #ifdef SIGTTOU
> /* Ignore this signal since it will happen when we try to set the
> pgrp. */
> @@ -497,7 +497,7 @@ child_terminal_ours_1 (int output_only)
> result = fcntl (0, F_SETFL, our_terminal_info.tflags);
> result = fcntl (0, F_SETFL, our_terminal_info.tflags);
> #endif
> - }
> + }
> }
again, another blank line.
> /* Per-inferior data key. */
> diff --git a/gdb/linux-record.c b/gdb/linux-record.c
> index 091ac8a..18f8fbf 100644
> --- a/gdb/linux-record.c
> +++ b/gdb/linux-record.c
> @@ -112,7 +112,7 @@ record_linux_sockaddr (struct regcache *regcache,
> "memory at addr = 0x%s len = %d.\n",
> phex_nz (len, tdep->size_pointer),
> tdep->size_int);
> - return -1;
> + return -1;
> }
> addrlen = (int) extract_unsigned_integer (a, tdep->size_int, byte_order);
> if (addrlen <= 0 || addrlen > tdep->size_sockaddr)
[...snip...]
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;
> ====
>
> And for glibc:
>
> diff --git a/stdio-common/vfscanf.c b/stdio-common/vfscanf.c
> index 1382eb5..1963c31 100644
> --- a/stdio-common/vfscanf.c
> +++ b/stdio-common/vfscanf.c
> @@ -1711,7 +1711,7 @@ _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
>
> /* The last thousands character will be added back by
> the char_buffer_add below. */
> - --charbuf.current;
> + --charbuf.current;
> #endif
> }
> else
Another one where there's a blank line, which 1/4 would have suppressed.
> diff --git a/stdlib/strtol_l.c b/stdlib/strtol_l.c
> index 8f6163d..0fb9a92 100644
> --- a/stdlib/strtol_l.c
> +++ b/stdlib/strtol_l.c
> @@ -351,8 +351,8 @@ INTERNAL (__strtol_l) (const STRING_TYPE *nptr, STRING_TYPE **endptr,
> && (wchar_t) c != thousands
> # else
> && ({ for (cnt = 0; cnt < thousands_len; ++cnt)
> - if (thousands[cnt] != end[cnt])
> - break;
> + if (thousands[cnt] != end[cnt])
> + break;
> cnt < thousands_len; })
> # endif
> && (!ISALPHA (c)
This is the one I filed earlier today as PR 68187.
> 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.
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
Dave
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall
2015-11-02 19:35 ` David Malcolm
@ 2015-11-02 20:35 ` Marc Glisse
2015-11-02 23:41 ` Jeff Law
1 sibling, 0 replies; 44+ messages in thread
From: Marc Glisse @ 2015-11-02 20:35 UTC (permalink / raw)
To: David Malcolm; +Cc: Patrick Palka, Richard Biener, Jeff Law, GCC Patches
On Mon, 2 Nov 2015, David Malcolm wrote:
> 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?)
You mean something like -Wstrict-overflow=n?
--
Marc Glisse
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall
2015-11-02 19:35 ` David Malcolm
2015-11-02 20:35 ` Marc Glisse
@ 2015-11-02 23:41 ` Jeff Law
2015-12-09 15:19 ` [PATCH] Add levels to -Wmisleading-indentation; add level 1 " David Malcolm
1 sibling, 1 reply; 44+ messages in thread
From: Jeff Law @ 2015-11-02 23:41 UTC (permalink / raw)
To: David Malcolm, Patrick Palka; +Cc: Richard Biener, GCC Patches
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.
Jeff
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall
2015-11-02 23:41 ` Jeff Law
@ 2015-12-09 15:19 ` David Malcolm
2015-12-09 15:40 ` Bernd Schmidt
0 siblings, 1 reply; 44+ messages in thread
From: David Malcolm @ 2015-12-09 15:19 UTC (permalink / raw)
To: Jeff Law; +Cc: Patrick Palka, Richard Biener, GCC Patches, David Malcolm
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall
2015-12-09 15:19 ` [PATCH] Add levels to -Wmisleading-indentation; add level 1 " David Malcolm
@ 2015-12-09 15:40 ` Bernd Schmidt
2015-12-09 16:49 ` David Malcolm
0 siblings, 1 reply; 44+ messages in thread
From: Bernd Schmidt @ 2015-12-09 15:40 UTC (permalink / raw)
To: David Malcolm, Jeff Law; +Cc: Patrick Palka, Richard Biener, GCC Patches
On 12/09/2015 04:38 PM, David Malcolm wrote:
> +/* 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);
> +
I personally see no use for the blank line heuristic, in fact I think it
is misguided. To my eyes a warning is clearly warranted for the examples
in this testcase. Do we actually have any users calling for this heuristic?
Bernd
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall
2015-12-09 15:40 ` Bernd Schmidt
@ 2015-12-09 16:49 ` David Malcolm
2015-12-09 17:19 ` Pedro Alves
2015-12-09 17:34 ` Bernd Schmidt
0 siblings, 2 replies; 44+ messages in thread
From: David Malcolm @ 2015-12-09 16:49 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: Jeff Law, Patrick Palka, Richard Biener, GCC Patches
On Wed, 2015-12-09 at 16:40 +0100, Bernd Schmidt wrote:
> On 12/09/2015 04:38 PM, David Malcolm wrote:
> > +/* 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);
> > +
>
> I personally see no use for the blank line heuristic, in fact I think it
> is misguided. To my eyes a warning is clearly warranted for the examples
> in this testcase.
This goes back to the examples given in:
https://gcc.gnu.org/ml/gcc-patches/2015-10/msg03225.html
This is about managing the signal:noise ratio for something in -Wall.
The distinction I want to make here is between badly indented code vs
misleadingly indented code. Yes, the code is badly indented, but to my
eyes the code is sufficiently badly indented that it has become *not*
misleading: I can immediately see that something is awry. I want to
preserve the -Wall level of the warning for the cases when it's not
immediately clear that there's a problem.
The levels idea means that you can turn off the blank-lines heuristic by
setting -Wmisleading-indentation=2.
> Do we actually have any users calling for this heuristic?
FWIW, gcc trunk is clean with the blank-lines heuristic, but not without
it (see the URL above for the 3 places I know of that fire without the
heuristic).
Dave
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall
2015-12-09 16:49 ` David Malcolm
@ 2015-12-09 17:19 ` Pedro Alves
2015-12-09 17:34 ` Bernd Schmidt
1 sibling, 0 replies; 44+ messages in thread
From: Pedro Alves @ 2015-12-09 17:19 UTC (permalink / raw)
To: David Malcolm, Bernd Schmidt
Cc: Jeff Law, Patrick Palka, Richard Biener, GCC Patches
On 12/09/2015 04:49 PM, David Malcolm wrote:
> This is about managing the signal:noise ratio for something in -Wall.
>
> The distinction I want to make here is between badly indented code vs
> misleadingly indented code. Yes, the code is badly indented, but to my
> eyes the code is sufficiently badly indented that it has become *not*
> misleading: I can immediately see that something is awry. I want to
> preserve the -Wall level of the warning for the cases when it's not
> immediately clear that there's a problem.
>
> The levels idea means that you can turn off the blank-lines heuristic by
> setting -Wmisleading-indentation=2.
IMHO, the problem with the levels idea will be if/when later you
come up with some other orthogonal heuristic to catch some other
class of indentation problem, and users want to enable it but
not the blank-lines heuristic, or vice-versa.
Also, levels don't allow finer-selection
with "-Werror -Wno-error=misleading-indentation", IIUC.
Did you consider -Wmisleading-indentation=blank-lines,foo,bar
instead, or totally separate switches like:
-Wmisleading-indentation
-Wmisleading-indentation-blank-lines
-Wmisleading-indentation-foo
-Wmisleading-indentation-bar
?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall
2015-12-09 16:49 ` David Malcolm
2015-12-09 17:19 ` Pedro Alves
@ 2015-12-09 17:34 ` Bernd Schmidt
1 sibling, 0 replies; 44+ messages in thread
From: Bernd Schmidt @ 2015-12-09 17:34 UTC (permalink / raw)
To: David Malcolm; +Cc: Jeff Law, Patrick Palka, Richard Biener, GCC Patches
On 12/09/2015 05:49 PM, David Malcolm wrote:
>>> +void
>>> +fn_40_implicit_level_1 (int arg)
>>> +{
>>> +if (flagA)
>>> + foo (0);
>>> +
>>> + foo (1);
>>> +
> The distinction I want to make here is between badly indented code vs
> misleadingly indented code. Yes, the code is badly indented, but to my
> eyes the code is sufficiently badly indented that it has become *not*
> misleading: I can immediately see that something is awry.
To my eyes and brain the effect of this code is disconcerting to the
point of confusion (and there was a case recently where a patch was
posted with a similar problem and it initially made me misinterpret the
code). So - if a human had seen this they'd have fixed it. Code like
this could have come in through a misapplied patch for example, and
you'd want to warn in such a case.
>> Do we actually have any users calling for this heuristic?
>
> FWIW, gcc trunk is clean with the blank-lines heuristic, but not without
> it (see the URL above for the 3 places I know of that fire without the
> heuristic).
That's a fairly low false-positive rate if we even want to call it that.
I'd fix the three examples and leave the warning code as is. We can
revisit the decision if we get a flood of bug reports.
Bernd
^ permalink raw reply [flat|nested] 44+ messages in thread
* [committed 1/5] Fix -Wmisleading-indentation warning in function.c
2015-10-29 17:42 ` Jeff Law
2015-10-30 9:16 ` Richard Biener
@ 2015-12-10 15:25 ` David Malcolm
2015-12-10 15:25 ` [committed 2/5] Fix misleading indentation in gcc/fortran/io.c David Malcolm
` (4 more replies)
2015-12-10 18:25 ` [PATCH 4/4] Add -Wmisleading-indentation to -Wall David Malcolm
2 siblings, 5 replies; 44+ messages in thread
From: David Malcolm @ 2015-12-10 15:25 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
Fix this warning:
../../../src/gcc/function.c: In function âvoid locate_and_pad_parm(machine_mode, tree, int, int, int, tree, args_size*, locate_and_pad_arg_data*)â:
../../../src/gcc/function.c:4123:2: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation]
{
^
../../../src/gcc/function.c:4119:7: note: ...this âifâ clause, but it is not
if (initial_offset_ptr->var)
^
[This warning would have been suppressed by the blank-lines heuristic
from:
https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01011.html ]
Preapproved by Jeff in:
https://gcc.gnu.org/ml/gcc-patches/2015-10/msg03246.html
Bootstrapped®rtested on x86_64-pc-linux-gnu.
Committed to trunk as r231518.
gcc/ChangeLog:
* function.c (locate_and_pad_parm): Fix indentation.
---
gcc/function.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/gcc/function.c b/gcc/function.c
index b513ead..035a49e 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -4181,14 +4181,14 @@ locate_and_pad_parm (machine_mode passed_mode, tree type, int in_regs,
locate->slot_offset.var = size_binop (MINUS_EXPR, ssize_int (0),
initial_offset_ptr->var);
- {
- tree s2 = sizetree;
- if (where_pad != none
- && (!tree_fits_uhwi_p (sizetree)
- || (tree_to_uhwi (sizetree) * BITS_PER_UNIT) % round_boundary))
- s2 = round_up (s2, round_boundary / BITS_PER_UNIT);
- SUB_PARM_SIZE (locate->slot_offset, s2);
- }
+ {
+ tree s2 = sizetree;
+ if (where_pad != none
+ && (!tree_fits_uhwi_p (sizetree)
+ || (tree_to_uhwi (sizetree) * BITS_PER_UNIT) % round_boundary))
+ s2 = round_up (s2, round_boundary / BITS_PER_UNIT);
+ SUB_PARM_SIZE (locate->slot_offset, s2);
+ }
locate->slot_offset.constant += part_size_in_regs;
--
1.8.5.3
^ permalink raw reply [flat|nested] 44+ messages in thread
* [committed 5/5] Fix -Wmisleading-indentation warning in graphite-optimize-isl.c
2015-12-10 15:25 ` [committed 1/5] Fix -Wmisleading-indentation warning in function.c David Malcolm
` (2 preceding siblings ...)
2015-12-10 15:25 ` [committed 3/5] Fix -Wmisleading-indentation warning in gcc/regrename.c David Malcolm
@ 2015-12-10 15:25 ` David Malcolm
2015-12-11 16:44 ` Sebastian Pop
2015-12-10 15:33 ` [committed 1/5] Fix -Wmisleading-indentation warning in function.c David Malcolm
4 siblings, 1 reply; 44+ messages in thread
From: David Malcolm @ 2015-12-10 15:25 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
../../../src/gcc/graphite-optimize-isl.c: In function ‘isl_union_set* scop_get_domains(scop_p)’:
../../../src/gcc/graphite-optimize-isl.c:362:5: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation]
return res;
^~~~~~
In file included from ../../../src/gcc/hash-table.h:236:0,
from ../../../src/gcc/coretypes.h:348,
from ../../../src/gcc/graphite-optimize-isl.c:28:
../../../src/gcc/vec.h:1343:3: note: ...this ‘for’ clause, but it is not
for (I = 0; (V).iterate ((I), &(P)); ++(I))
^
../../../src/gcc/graphite-optimize-isl.c:359:3: note: in expansion of macro ‘FOR_EACH_VEC_ELT’
FOR_EACH_VEC_ELT (scop->pbbs, i, pbb)
^~~~~~~~~~~~~~~~
351 static isl_union_set *
352 scop_get_domains (scop_p scop ATTRIBUTE_UNUSED)
353 {
354 int i;
355 poly_bb_p pbb;
356 isl_space *space = isl_set_get_space (scop->param_context);
357 isl_union_set *res = isl_union_set_empty (space);
358
359 FOR_EACH_VEC_ELT (scop->pbbs, i, pbb)
360 res = isl_union_set_add_set (res, isl_set_copy (pbb->domain));
361
362 return res;
^ warning is here
363 }
[This warning would have been suppressed by the blank-lines heuristic
from:
https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01011.html ]
Preapproved by Jeff in:
https://gcc.gnu.org/ml/gcc-patches/2015-10/msg03246.html
Bootstrapped®rtested on x86_64-pc-linux-gnu.
Committed to trunk as r231522.
gcc/ChangeLog:
* graphite-optimize-isl.c (scop_get_domains): Fix indentation.
---
gcc/graphite-optimize-isl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/graphite-optimize-isl.c b/gcc/graphite-optimize-isl.c
index 8727e39..0c6a971 100644
--- a/gcc/graphite-optimize-isl.c
+++ b/gcc/graphite-optimize-isl.c
@@ -359,7 +359,7 @@ scop_get_domains (scop_p scop ATTRIBUTE_UNUSED)
FOR_EACH_VEC_ELT (scop->pbbs, i, pbb)
res = isl_union_set_add_set (res, isl_set_copy (pbb->domain));
- return res;
+ return res;
}
static const int CONSTANT_BOUND = 20;
--
1.8.5.3
^ permalink raw reply [flat|nested] 44+ messages in thread
* [committed 2/5] Fix misleading indentation in gcc/fortran/io.c
2015-12-10 15:25 ` [committed 1/5] Fix -Wmisleading-indentation warning in function.c David Malcolm
@ 2015-12-10 15:25 ` David Malcolm
2015-12-10 15:25 ` [committed 4/5] Fix -Wmisleading-indentation warning in ifcvt.c David Malcolm
` (3 subsequent siblings)
4 siblings, 0 replies; 44+ messages in thread
From: David Malcolm @ 2015-12-10 15:25 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
Fix this warning:
../../../src/gcc/fortran/io.c: In function âmatch gfc_match_open()â:
../../../src/gcc/fortran/io.c:2003:4: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation]
if (!compare_to_allowed_values ("DELIM", delim, NULL, NULL,
^
../../../src/gcc/fortran/io.c:2000:2: note: ...this âifâ clause, but it is not
if (!is_char_type ("DELIM", open->delim))
^
[This warning would have been suppressed by the blank-lines heuristic
from:
https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01011.html ]
Preapproved by Jeff in:
https://gcc.gnu.org/ml/gcc-patches/2015-10/msg03246.html
Bootstrapped®rtested on x86_64-pc-linux-gnu.
Committed to trunk as r231519.
gcc/fortran/ChangeLog:
* io.c (gfc_match_open): Fix indentation.
---
gcc/fortran/io.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c
index 8cf952f..18816f2 100644
--- a/gcc/fortran/io.c
+++ b/gcc/fortran/io.c
@@ -2006,8 +2006,8 @@ gfc_match_open (void)
{
static const char *delim[] = { "APOSTROPHE", "QUOTE", "NONE", NULL };
- if (!is_char_type ("DELIM", open->delim))
- goto cleanup;
+ if (!is_char_type ("DELIM", open->delim))
+ goto cleanup;
if (!compare_to_allowed_values ("DELIM", delim, NULL, NULL,
open->delim->value.character.string,
--
1.8.5.3
^ permalink raw reply [flat|nested] 44+ messages in thread
* [committed 3/5] Fix -Wmisleading-indentation warning in gcc/regrename.c
2015-12-10 15:25 ` [committed 1/5] Fix -Wmisleading-indentation warning in function.c David Malcolm
2015-12-10 15:25 ` [committed 2/5] Fix misleading indentation in gcc/fortran/io.c David Malcolm
2015-12-10 15:25 ` [committed 4/5] Fix -Wmisleading-indentation warning in ifcvt.c David Malcolm
@ 2015-12-10 15:25 ` David Malcolm
2015-12-10 15:25 ` [committed 5/5] Fix -Wmisleading-indentation warning in graphite-optimize-isl.c David Malcolm
2015-12-10 15:33 ` [committed 1/5] Fix -Wmisleading-indentation warning in function.c David Malcolm
4 siblings, 0 replies; 44+ messages in thread
From: David Malcolm @ 2015-12-10 15:25 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
../../../src/gcc/regrename.c: In function âvoid scan_rtx_address(rtx_insn*, rtx_def**, reg_class, scan_actions, machine_mode, addr_space_t)â:
../../../src/gcc/regrename.c:1314:7: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation]
break;
^
../../../src/gcc/regrename.c:1311:5: note: ...this âifâ clause, but it is not
if (!AUTO_INC_DEC)
^
[This warning would have been suppressed by the blank-lines heuristic
from:
https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01011.html ]
Preapproved by Jeff in:
https://gcc.gnu.org/ml/gcc-patches/2015-10/msg03246.html
Bootstrapped®rtested on x86_64-pc-linux-gnu.
Committed to trunk as r231520.
gcc/ChangeLog:
* regrename.c (scan_rtx_address): Fix indentation.
---
gcc/regrename.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gcc/regrename.c b/gcc/regrename.c
index 701e078..60157dc 100644
--- a/gcc/regrename.c
+++ b/gcc/regrename.c
@@ -1360,8 +1360,8 @@ scan_rtx_address (rtx_insn *insn, rtx *loc, enum reg_class cl,
case PRE_MODIFY:
/* If the target doesn't claim to handle autoinc, this must be
something special, like a stack push. Kill this chain. */
- if (!AUTO_INC_DEC)
- action = mark_all_read;
+ if (!AUTO_INC_DEC)
+ action = mark_all_read;
break;
--
1.8.5.3
^ permalink raw reply [flat|nested] 44+ messages in thread
* [committed 4/5] Fix -Wmisleading-indentation warning in ifcvt.c
2015-12-10 15:25 ` [committed 1/5] Fix -Wmisleading-indentation warning in function.c David Malcolm
2015-12-10 15:25 ` [committed 2/5] Fix misleading indentation in gcc/fortran/io.c David Malcolm
@ 2015-12-10 15:25 ` David Malcolm
2015-12-10 15:25 ` [committed 3/5] Fix -Wmisleading-indentation warning in gcc/regrename.c David Malcolm
` (2 subsequent siblings)
4 siblings, 0 replies; 44+ messages in thread
From: David Malcolm @ 2015-12-10 15:25 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
../../../src/gcc/ifcvt.c: In function âbool noce_try_inverse_constants(noce_if_info*)â:
../../../src/gcc/ifcvt.c:1233:2: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation]
seq = end_ifcvt_sequence (if_info);
^~~
../../../src/gcc/ifcvt.c:1230:7: note: ...this âifâ clause, but it is not
if (target != if_info->x)
^~
due to the lack of an outdent after the conditional at lines 1230-1:
1220 if (target)
1221 {
1222 rtx_insn *seq = get_insns ();
1223
1224 if (!seq)
1225 {
1226 end_sequence ();
1227 return false;
1228 }
1229
>>1230 if (target != if_info->x)
>>1231 noce_emit_move_insn (if_info->x, target);
1232
>>1233 seq = end_ifcvt_sequence (if_info);
1234
1235 if (!seq)
1236 return false;
1237
1238 emit_insn_before_setloc (seq, if_info->jump,
1239 INSN_LOCATION (if_info->insn_a));
1240 return true;
1241 }
[This warning would have been suppressed by the blank-lines heuristic
from:
https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01011.html ]
Preapproved by Jeff in:
https://gcc.gnu.org/ml/gcc-patches/2015-10/msg03246.html
Bootstrapped®rtested on x86_64-pc-linux-gnu.
Committed to trunk as r231521.
gcc/ChangeLog:
* ifcvt.c (noce_try_inverse_constants): Fix indentation.
---
gcc/ifcvt.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index d474b3b..7fb1dab 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -1230,14 +1230,14 @@ noce_try_inverse_constants (struct noce_if_info *if_info)
if (target != if_info->x)
noce_emit_move_insn (if_info->x, target);
- seq = end_ifcvt_sequence (if_info);
+ seq = end_ifcvt_sequence (if_info);
- if (!seq)
- return false;
+ if (!seq)
+ return false;
- emit_insn_before_setloc (seq, if_info->jump,
- INSN_LOCATION (if_info->insn_a));
- return true;
+ emit_insn_before_setloc (seq, if_info->jump,
+ INSN_LOCATION (if_info->insn_a));
+ return true;
}
end_sequence ();
--
1.8.5.3
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [committed 1/5] Fix -Wmisleading-indentation warning in function.c
2015-12-10 15:25 ` [committed 1/5] Fix -Wmisleading-indentation warning in function.c David Malcolm
` (3 preceding siblings ...)
2015-12-10 15:25 ` [committed 5/5] Fix -Wmisleading-indentation warning in graphite-optimize-isl.c David Malcolm
@ 2015-12-10 15:33 ` David Malcolm
4 siblings, 0 replies; 44+ messages in thread
From: David Malcolm @ 2015-12-10 15:33 UTC (permalink / raw)
To: gcc-patches
FWIW, I believe trunk is clean for -Wmisleading-indentation after these
5 patches (for x86_64-pc-linux-gnu, at least).
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall
2015-10-29 17:42 ` Jeff Law
2015-10-30 9:16 ` Richard Biener
2015-12-10 15:25 ` [committed 1/5] Fix -Wmisleading-indentation warning in function.c David Malcolm
@ 2015-12-10 18:25 ` David Malcolm
2 siblings, 0 replies; 44+ messages in thread
From: David Malcolm @ 2015-12-10 18:25 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
On Thu, 2015-10-29 at 11:38 -0600, Jeff Law wrote:
> On 10/29/2015 10:49 AM, David Malcolm wrote:
> > Our documentation describes -Wall as enabling "all the warnings about
> > constructions that some users consider questionable, and that are easy to avoid
> > (or modify to prevent the warning), even in conjunction with macros."
> >
> > I believe that -Wmisleading-indentation meets these criteria, and is
> > likely to be of benefit to users who may not read release notes; it
> > warns for indentation that's misleading, but not for indentation
> > that's merely bad: the former are places where a user will likely
> > want to fix the code.
> >
> > The fix is usually easy and obvious: fix the misleadingly-indented
> > code. If that isn't an option for some reason, pragmas can be used to
> > turn off the warning for a particular fragment of code:
> >
> > #pragma GCC diagnostic push
> > #pragma GCC diagnostic ignored "-Wmisleading-indentation"
> > if (flag)
> > x = 3;
> > y = 2;
> > #pragma GCC diagnostic pop
> >
> > -Wmisleading-indentation has been tested with a variety of indentation
> > styles (see gcc/testsuite/c-c++-common/Wmisleading-indentation.c)
> > and on a variety of real-world projects. For example, in:
> > https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg119790.html
> > Patrick reports:
> > "Tested by building the linux, git, vim, sqlite and gdb-binutils sources
> > with -Wmisleading-indentation."
> >
> > With the tweak earlier in this kit I believe we now have a good
> > enough signal:noise ratio for this warning to be widely used; hence this
> > patch adds the warning to -Wall.
> >
> > Bootstrapped®rtested with x86_64-pc-linux-gnu.
> >
> > OK for trunk?
> >
> > gcc/c-family/ChangeLog:
> > * c.opt (Wmisleading-indentation): Add to -Wall for C and C++.
> >
> > gcc/ChangeLog:
> > * doc/invoke.texi (-Wall): Add -Wmisleading-indentation to the
> > list.
> > (-Wmisleading-indentation): Update documentation to reflect
> > being enabled by -Wall in C/C++.
> I'm sure we'll get some grief for this :-)
>
> Approved once we're clean in GCC. I'm going to explicitly say that
> we'll have to watch for fallout, particularly as we start getting
> feedback from Debian & Fedora mass-rebuilds as we approach release time.
> If the fallout is too bad, we'll have to reconsider.
I believe we're now clean; I've committed this to trunk (*without* the
blank-lines heuristic/levels idea) as r231539, having
bootstrapped®rtested on x86_64-pc-linux-gnu (I've also successfully
performed an all-configs build with it, although that was a while back
now).
> I'll pre-approve patches which fix anything caught by this option in GCC
> as long as the fix just adjusts whitespace :-)
>
> jeff
>
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [committed 5/5] Fix -Wmisleading-indentation warning in graphite-optimize-isl.c
2015-12-10 15:25 ` [committed 5/5] Fix -Wmisleading-indentation warning in graphite-optimize-isl.c David Malcolm
@ 2015-12-11 16:44 ` Sebastian Pop
0 siblings, 0 replies; 44+ messages in thread
From: Sebastian Pop @ 2015-12-11 16:44 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches
David Malcolm wrote:
> gcc/ChangeLog:
> * graphite-optimize-isl.c (scop_get_domains): Fix indentation.
> ---
> gcc/graphite-optimize-isl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/graphite-optimize-isl.c b/gcc/graphite-optimize-isl.c
> index 8727e39..0c6a971 100644
> --- a/gcc/graphite-optimize-isl.c
> +++ b/gcc/graphite-optimize-isl.c
> @@ -359,7 +359,7 @@ scop_get_domains (scop_p scop ATTRIBUTE_UNUSED)
> FOR_EACH_VEC_ELT (scop->pbbs, i, pbb)
> res = isl_union_set_add_set (res, isl_set_copy (pbb->domain));
>
> - return res;
> + return res;
Thanks!
Sebastian
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall
2015-12-11 11:51 ` Iain Sandoe
@ 2015-12-11 16:48 ` Dominique d'Humières
0 siblings, 0 replies; 44+ messages in thread
From: Dominique d'Humières @ 2015-12-11 16:48 UTC (permalink / raw)
To: Iain Sandoe; +Cc: jbglaw, dmalcolm, law, gcc-patches
Revision r231571 with Jan-Benedict Glaw’s fix for trailing whitespace.
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog (revision 231570)
+++ gcc/ChangeLog (working copy)
@@ -1,3 +1,13 @@
+2015-12-11 Jan-Benedict Glaw <jbglaw@lug-owl.de>
+ Dominique d'Humieres <dominiq@lps.ens.fr>
+
+ PR target/26427
+ PR target/33120
+ PR testsuite/35710
+
+ * config/darwin.c (darwin_use_anchors_for_symbol_p): Fix indention and
+ trailing whitespace.
+
2015-12-11 Jan Beulich <jbeulich@suse.com>
* cfgexpand.c (expand_one_var): Exit early for static and
Index: gcc/config/darwin.c
===================================================================
--- gcc/config/darwin.c (revision 231570)
+++ gcc/config/darwin.c (working copy)
@@ -2997,23 +2997,23 @@
SYMBOL_REF_BLOCK_OFFSET (symbol));
}
-/* Disable section anchoring on any section containing a zero-sized
+/* Disable section anchoring on any section containing a zero-sized
object. */
bool
darwin_use_anchors_for_symbol_p (const_rtx symbol)
{
- if (DARWIN_SECTION_ANCHORS && flag_section_anchors)
+ if (DARWIN_SECTION_ANCHORS && flag_section_anchors)
{
section *sect;
/* If the section contains a zero-sized object it's ineligible. */
sect = SYMBOL_REF_BLOCK (symbol)->sect;
/* This should have the effect of disabling anchors for vars that follow
- any zero-sized one, in a given section. */
+ any zero-sized one, in a given section. */
if (sect->common.flags & SECTION_NO_ANCHOR)
return false;
- /* Also check the normal reasons for suppressing. */
- return default_use_anchors_for_symbol_p (symbol);
+ /* Also check the normal reasons for suppressing. */
+ return default_use_anchors_for_symbol_p (symbol);
}
else
return false;
Dominique
> I think you can apply this as obvious
> Iain
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall
2015-12-11 11:26 [PATCH 4/4] Add -Wmisleading-indentation to -Wall Dominique d'Humières
@ 2015-12-11 11:51 ` Iain Sandoe
2015-12-11 16:48 ` Dominique d'Humières
0 siblings, 1 reply; 44+ messages in thread
From: Iain Sandoe @ 2015-12-11 11:51 UTC (permalink / raw)
To: Dominique Dhumieres; +Cc: dmalcolm, law, gcc-patches
> On 11 Dec 2015, at 11:25, Dominique d'Humières <dominiq@lps.ens.fr> wrote:
>
> This breaks bootstrap on darwin:
>
> ../../work/gcc/config/darwin.c: In function 'bool darwin_use_anchors_for_symbol_p(const_rtx)':
> ../../work/gcc/config/darwin.c:3016:9: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation]
> return default_use_anchors_for_symbol_p (symbol);
> ^~~~~~
>
> ../../work/gcc/config/darwin.c:3012:7: note: ...this 'if' clause, but it is not
> if (sect->common.flags & SECTION_NO_ANCHOR)
> ^~
>
> cc1plus: all warnings being treated as errors
>
> Fixed by the following patch
>
> --- ../_clean/gcc/config/darwin.c 2015-10-16 22:46:35.000000000 +0200
> +++ gcc/config/darwin.c 2015-12-11 12:17:40.000000000 +0100
> @@ -3012,8 +3012,8 @@ darwin_use_anchors_for_symbol_p (const_r
> if (sect->common.flags & SECTION_NO_ANCHOR)
> return false;
>
> - /* Also check the normal reasons for suppressing. */
> - return default_use_anchors_for_symbol_p (symbol);
> + /* Also check the normal reasons for suppressing. */
> + return default_use_anchors_for_symbol_p (symbol);
> }
> else
> return false;
I think you can apply this as obvious
Iain
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall
@ 2015-12-11 11:26 Dominique d'Humières
2015-12-11 11:51 ` Iain Sandoe
0 siblings, 1 reply; 44+ messages in thread
From: Dominique d'Humières @ 2015-12-11 11:26 UTC (permalink / raw)
To: dmalcolm; +Cc: law, Iain Sandoe, gcc-patches
This breaks bootstrap on darwin:
../../work/gcc/config/darwin.c: In function 'bool darwin_use_anchors_for_symbol_p(const_rtx)':
../../work/gcc/config/darwin.c:3016:9: error: statement is indented as if it were guarded by... [-Werror=misleading-indentation]
return default_use_anchors_for_symbol_p (symbol);
^~~~~~
../../work/gcc/config/darwin.c:3012:7: note: ...this 'if' clause, but it is not
if (sect->common.flags & SECTION_NO_ANCHOR)
^~
cc1plus: all warnings being treated as errors
Fixed by the following patch
--- ../_clean/gcc/config/darwin.c 2015-10-16 22:46:35.000000000 +0200
+++ gcc/config/darwin.c 2015-12-11 12:17:40.000000000 +0100
@@ -3012,8 +3012,8 @@ darwin_use_anchors_for_symbol_p (const_r
if (sect->common.flags & SECTION_NO_ANCHOR)
return false;
- /* Also check the normal reasons for suppressing. */
- return default_use_anchors_for_symbol_p (symbol);
+ /* Also check the normal reasons for suppressing. */
+ return default_use_anchors_for_symbol_p (symbol);
}
else
return false;
TIA
Dominique
^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2015-12-11 16:48 UTC | newest]
Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-29 16:32 [PATCH 0/4] -Wmisleading-indentation David Malcolm
2015-10-29 16:32 ` [PATCH 3/4] Fix misleading indentation in tree-nested.c David Malcolm
2015-10-29 17:36 ` Jeff Law
2015-10-29 16:32 ` [PATCH 4/4] Add -Wmisleading-indentation to -Wall David Malcolm
2015-10-29 17:42 ` Jeff Law
2015-10-30 9:16 ` Richard Biener
2015-11-01 22:06 ` Patrick Palka
2015-11-02 16:21 ` David Malcolm
2015-11-02 17:11 ` David Malcolm
2015-11-02 18:39 ` Patrick Palka
2015-11-02 19:35 ` David Malcolm
2015-11-02 20:35 ` Marc Glisse
2015-11-02 23:41 ` Jeff Law
2015-12-09 15:19 ` [PATCH] Add levels to -Wmisleading-indentation; add level 1 " David Malcolm
2015-12-09 15:40 ` Bernd Schmidt
2015-12-09 16:49 ` David Malcolm
2015-12-09 17:19 ` Pedro Alves
2015-12-09 17:34 ` Bernd Schmidt
2015-12-10 15:25 ` [committed 1/5] Fix -Wmisleading-indentation warning in function.c David Malcolm
2015-12-10 15:25 ` [committed 2/5] Fix misleading indentation in gcc/fortran/io.c David Malcolm
2015-12-10 15:25 ` [committed 4/5] Fix -Wmisleading-indentation warning in ifcvt.c David Malcolm
2015-12-10 15:25 ` [committed 3/5] Fix -Wmisleading-indentation warning in gcc/regrename.c David Malcolm
2015-12-10 15:25 ` [committed 5/5] Fix -Wmisleading-indentation warning in graphite-optimize-isl.c David Malcolm
2015-12-11 16:44 ` Sebastian Pop
2015-12-10 15:33 ` [committed 1/5] Fix -Wmisleading-indentation warning in function.c David Malcolm
2015-12-10 18:25 ` [PATCH 4/4] Add -Wmisleading-indentation to -Wall David Malcolm
2015-10-29 16:32 ` [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines David Malcolm
2015-10-29 17:33 ` Jeff Law
2015-10-29 17:38 ` Patrick Palka
2015-10-29 17:44 ` Patrick Palka
2015-10-29 17:50 ` Jeff Law
2015-10-29 17:56 ` Mike Stump
2015-10-29 17:56 ` Patrick Palka
2015-10-29 18:00 ` Mike Stump
2015-10-29 18:13 ` David Malcolm
2015-10-29 17:59 ` AW: " bernds_cb1
2015-10-29 16:32 ` [PATCH 2/4] Fix misleading indentation in tree-ssa-loop-unswitch.c David Malcolm
2015-10-29 17:34 ` Jeff Law
2015-10-30 5:42 ` [PATCH 0/4] -Wmisleading-indentation Andi Kleen
2015-10-30 5:59 ` Jeff Law
2015-10-30 15:42 ` Mike Stump
2015-12-11 11:26 [PATCH 4/4] Add -Wmisleading-indentation to -Wall Dominique d'Humières
2015-12-11 11:51 ` Iain Sandoe
2015-12-11 16:48 ` Dominique d'Humières
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).