[PATCH 0/4] -Wmisleading-indentation

[PATCH 0/4] -Wmisleading-indentation

[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&regrtested 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

[PATCH 2/4] Fix misleading indentation in tree-ssa-loop-unswitch.c

[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

Re: [PATCH 2/4] Fix misleading indentation in tree-ssa-loop-unswitch.c

[Jeff Law]

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

[PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines

[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

Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines

[Jeff Law]

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

Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines

[Patrick Palka]

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?

Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines

[Patrick Palka]

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 :)

Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines

[Jeff Law]

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
>

Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines

[Mike Stump]

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.

Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines

[Patrick Palka]

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.)

Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines

[Mike Stump]

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.

Re: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines

[David Malcolm]

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.

AW: [PATCH 1/4] -Wmisleading-indentation: don't warn in presence of entirely blank lines

[bernds_cb1]

-----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

[PATCH 4/4] Add -Wmisleading-indentation to -Wall

[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&regrtested 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

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[Jeff Law]

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&regrtested 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

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[Richard Biener]

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&regrtested 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
>
>

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[Patrick Palka]

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&regrtested 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.)

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[David Malcolm]

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&regrtested 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

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[David Malcolm]

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&regrtested 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.

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[Patrick Palka]

[-- 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&regrtested 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;

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[David Malcolm]

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&regrtested 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

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[Marc Glisse]

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

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[Jeff Law]

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

[PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall

[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&regrtested 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

Re: [PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall

[Bernd Schmidt]

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

Re: [PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall

[David Malcolm]

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

Re: [PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall

[Pedro Alves]

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

?

Re: [PATCH] Add levels to -Wmisleading-indentation; add level 1 to -Wall

[Bernd Schmidt]

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

[committed 1/5] Fix -Wmisleading-indentation warning in function.c

[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&regrtested 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

[committed 3/5] Fix -Wmisleading-indentation warning in gcc/regrename.c

[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&regrtested 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

[committed 5/5] Fix -Wmisleading-indentation warning in graphite-optimize-isl.c

[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&regrtested 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

Re: [committed 5/5] Fix -Wmisleading-indentation warning in graphite-optimize-isl.c

[Sebastian Pop]

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

[committed 2/5] Fix misleading indentation in gcc/fortran/io.c

[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&regrtested 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

[committed 4/5] Fix -Wmisleading-indentation warning in ifcvt.c

[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&regrtested 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

Re: [committed 1/5] Fix -Wmisleading-indentation warning in function.c

[David Malcolm]

FWIW, I believe trunk is clean for -Wmisleading-indentation after these
5 patches (for x86_64-pc-linux-gnu, at least).

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[David Malcolm]

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&regrtested 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&regrtested 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
> 
> 

[PATCH 3/4] Fix misleading indentation in tree-nested.c

[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

Re: [PATCH 3/4] Fix misleading indentation in tree-nested.c

[Jeff Law]

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

Re: [PATCH 0/4] -Wmisleading-indentation

[Andi Kleen]

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

Re: [PATCH 0/4] -Wmisleading-indentation

[Jeff Law]

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

Re: [PATCH 0/4] -Wmisleading-indentation

[Mike Stump]

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).

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[Dominique d'Humières]

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

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[Iain Sandoe]

> 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

Re: [PATCH 4/4] Add -Wmisleading-indentation to -Wall

[Dominique d'Humières]

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