[PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0)

[PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0)

[David Malcolm]

PR c/68187 covers two cases involving poor indentation where
the indentation is arguably not misleading, but for which
-Wmisleading-indentation emits a warning.

The two cases appear to be different in nature; one in comment #0
and the other in comment #1.  Richi marked the bug as a whole as
a P1 regression; it's not clear to me if he meant one or both of
these cases, so the following two patches fix both.

The rest of this post addresses the case in comment #0 of the PR;
the followup post addresses the other case, in comment #1 of the PR.

Building glibc (a9224562cbe9cfb0bd8d9e637a06141141f9e6e3) on x86_64
led to this diagnostic from -Wmisleading-indentation:

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

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;

Lines 354 and 355 are poorly indented, leading to the warning from
-Wmisleading-indentation at line 356.

The wording of the warning is clearly wrong: line 356 isn't indented as if
guarded by line 353, it's more that lines 354 and 355 *aren't* indented.

What's happening is that should_warn_for_misleading_indentation has a
heuristic for handling "} else", such as:

     if (p)
       foo (1);
     } else       // GUARD
       foo (2);   // BODY
       foo (3);   // NEXT

and this heuristic uses the first non-whitespace character in the line
containing GUARD as the column of interest: the "}" character.

In this case we have:

   353        && ({ for (cnt = 0; cnt < thousands_len; ++cnt)  // GUARD
   354              if (thousands[cnt] != end[cnt])            // BODY
   355                break;
   356              cnt < thousands_len; })                    // NEXT

and so it uses the column of the "&&", and treats it as if it were
indented thus:

   353        for (cnt = 0; cnt < thousands_len; ++cnt)        // GUARD
   354              if (thousands[cnt] != end[cnt])            // BODY
   355                break;
   356              cnt < thousands_len; })                    // NEXT

and thus issues a warning.

As far as I can tell the heuristic in question only makes sense for
"else" clauses, so the following patch updates it to only use the
special column when handling "else" clauses, eliminating the
overzealous warning.

Doing so led to a nonsensical warning for
libstdc++-v3/src/c++11/random.cc:random_device::_M_init:

random.cc: In member function ‘void std::random_device::_M_init(const string&)’:
random.cc:102:10: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
     else if (token != "/dev/urandom" && token != "/dev/random")
          ^~
random.cc:107:5: note: ...this statement, but the latter is indented as if it does
     _M_file = static_cast<void*>(std::fopen(fname, "rb"));
     ^~~~~~~

so the patch addresses this by tweaking the heuristic that rejects
aligned BODY and NEXT so that it doesn't require them to be aligned with
the first non-whitespace of the GUARD, simply that they not be indented
relative to it.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu in
combination with the following patch; standalone bootstrap&regrtest
is in progress.

OK for trunk if the latter is successful?

gcc/c-family/ChangeLog:
	PR c/68187
	* c-indentation.c (should_warn_for_misleading_indentation): When
	suppressing warnings about cases where the guard and body are on
	the same column, only use the first non-whitespace column in place
	of the guard token column when dealing with "else" clauses.
	When rejecting aligned BODY and NEXT, loosen the requirement
	from equality with the first non-whitespace of guard to simply
	that they not be indented relative to it.

gcc/testsuite/ChangeLog:
	PR c/68187
	* c-c++-common/Wmisleading-indentation.c (fn_40_a): New test
	function.
	(fn_40_b): Likewise.
	(fn_41_a): Likewise.
	(fn_41_b): Likewise.
---
 gcc/c-family/c-indentation.c                       | 16 +++--
 .../c-c++-common/Wmisleading-indentation.c         | 79 ++++++++++++++++++++++
 2 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index 521f992..c72192d 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -419,7 +419,8 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 	{
           /* Don't warn if they are aligned on the same column
 	     as the guard itself (suggesting autogenerated code that doesn't
-	     bother indenting at all).  We consider the column of the first
+	     bother indenting at all).
+	     For "else" clauses, we consider the column of the first
 	     non-whitespace character on the guard line instead of the column
 	     of the actual guard token itself because it is more sensible.
 	     Consider:
@@ -438,14 +439,17 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 	       foo (2);   // BODY
 	       foo (3);   // NEXT
 
-	     If we just used the column of the guard token, we would warn on
+	     If we just used the column of the "else" token, we would warn on
 	     the first example and not warn on the second.  But we want the
 	     exact opposite to happen: to not warn on the first example (which
 	     is probably autogenerated) and to warn on the second (whose
 	     indentation is misleading).  Using the column of the first
 	     non-whitespace character on the guard line makes that
 	     happen.  */
-	  if (guard_line_first_nws == body_vis_column)
+	  unsigned int guard_column = (guard_tinfo.keyword == RID_ELSE
+				       ? guard_line_first_nws
+				       : guard_vis_column);
+	  if (guard_column == body_vis_column)
 	    return false;
 
 	  /* We may have something like:
@@ -458,9 +462,9 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 	     foo (3);  // NEXT
 
 	     in which case the columns are not aligned but the code is not
-	     misleadingly indented.  If the column of the body is less than
-	     that of the guard line then don't warn.  */
-	  if (body_vis_column < guard_line_first_nws)
+	     misleadingly indented.  If the column of the body isn't indented
+	     more than the guard line then don't warn.  */
+	  if (body_vis_column <= guard_line_first_nws)
 	    return false;
 
 	  /* Don't warn if there is multiline preprocessor logic between
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
index 25db8fe..04500b7 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
@@ -903,3 +903,82 @@ void pr69122 (void)
   emit foo (1);
 }
 #undef emit
+
+/* In the following, the 'if' within the 'for' statement is not indented,
+   but arguably should be.
+   The for loop:
+     "for (cnt = 0; cnt < thousands_len; ++cnt)"
+   does not guard this conditional:
+     "cnt < thousands_len;".
+   and the poor indentation is not misleading.  Verify that we do
+   not erroneously emit a warning about this.
+   Based on an example seen in glibc (PR c/68187).  */
+
+void
+fn_40_a (const char *end, const char *thousands, int thousands_len)
+{
+  int cnt;
+
+  while (flagA)
+    if (flagA
+        && ({ for (cnt = 0; cnt < thousands_len; ++cnt)
+              if (thousands[cnt] != end[cnt])
+                break;
+              cnt < thousands_len; })
+        && flagB)
+      break;
+}
+
+/* As above, but with the indentation within the "for" loop fixed.
+   We should not emit a warning for this, either.  */
+
+void
+fn_40_b (const char *end, const char *thousands, int thousands_len)
+{
+  int cnt;
+
+  while (flagA)
+    if (flagA
+        && ({ for (cnt = 0; cnt < thousands_len; ++cnt)
+                if (thousands[cnt] != end[cnt])
+                  break;
+              cnt < thousands_len; })
+        && flagB)
+      break;
+}
+
+/* We should not warn for the following
+   (based on libstdc++-v3/src/c++11/random.cc:random_device::_M_init).  */
+
+void
+fn_41_a (void)
+{
+  if (flagA)
+    {
+    }
+  else if (flagB)
+  fail:
+    foo (0);
+
+  foo (1);
+  if (!flagC)
+    goto fail;
+}
+
+/* Tweaked version of the above (with the label indented), which we should
+   also not warn for.  */
+
+void
+fn_41_b (void)
+{
+  if (flagA)
+    {
+    }
+  else if (flagB)
+   fail:
+    foo (0);
+
+  foo (1);
+  if (!flagC)
+    goto fail;
+}
-- 
1.8.5.3

[PATCH 2/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #1)

[David Malcolm]

Comment #1 of PR c/68187 identified another overzealous warning
from -Wmisleading-indentation, with OpenSSL 1.0.1, on this poorly
indented code:

115    if (locked)
116        i = CRYPTO_add(&e->struct_ref, -1, CRYPTO_LOCK_ENGINE);
117    else
118        i = --e->struct_ref;
119    engine_ref_debug(e, 0, -1)
120        if (i > 0)
121        return 1;

eng_lib.c:120:9: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
         if (i > 0)
         ^~
eng_lib.c:117:5: note: ...this 'else' clause, but it is not
     else
     ^~~~

Line 120 is poorly indented, but the warning is arguably unjustified.

Root cause is that "engine_ref_debug" is actually a debugging macro
that was empty in the given configuration, so the code effectively
was:

117    else                      // GUARD
118        i = --e->struct_ref;  // BODY
119
120        if (i > 0)            // NEXT

hence the warning.

But the code as seen by a human is clearly *not* misleading, and
hence arguably we shouldn't warn for this case.

The following patch fixes this by ruling that if there is non-whitespace
in a line between the BODY and the NEXT statements, and that this
non-whitespace is effectively an "unindent" or "outdent" (it's not clear
to me which of these terms is better), then to not issue a warning.

In doing so I eliminated one of the existing heuristics: we already
had code to ignore preprocessor directives between BODY and NEXT for cases
like this:

  if (flagA)  // GUARD
    foo (0);  // BODY
#if SOME_CONDITION_THAT_DOES_NOT_HOLD
  if (flagB)
#endif
    foo (1);  // NEXT

This is handled by the new heuristic, so the new heuristic simply
replaces the old one.  Sadly the replacement of two old functions
with two new functions leads to a rather messy diff within
c-indentation.c; I can split it up into a removal/addition pair of
patches if that's easier to review.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu (in
combination with the previous patch).

OK for trunk?

Note: one of the new test cases adds a dg-warning/dg-message pair, and so
would require updating if/when the wording change posted here:
  https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00068.html
  ("[PATCH] PR c/69993: improvements to wording of -Wmisleading-indentation")
is applied.

gcc/c-family/ChangeLog:
	PR c/68187
	* c-indentation.c (get_visual_column): Move code to determine next
	tab stop to...
	(next_tab_stop): ...this new function.
	(line_contains_hash_if): Delete function.
	(detect_preprocessor_logic): Delete function.
	(get_first_nws_vis_column): New function.
	(detect_intervening_unindent): New function.
	(should_warn_for_misleading_indentation): Replace call to
	detect_preprocessor_logic with a call to
	detect_intervening_unindent.

gcc/testsuite/ChangeLog:
	PR c/68187
	* c-c++-common/Wmisleading-indentation.c (fn_42_a): New test
	function.
	(fn_42_b): Likewise.
	(fn_42_c): Likewise.
---
 gcc/c-family/c-indentation.c                       | 141 ++++++++++++---------
 .../c-c++-common/Wmisleading-indentation.c         |  72 +++++++++++
 2 files changed, 152 insertions(+), 61 deletions(-)

diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index c72192d..b84fbf4 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -26,6 +26,16 @@ along with GCC; see the file COPYING3.  If not see
 
 extern cpp_options *cpp_opts;
 
+/* Round up VIS_COLUMN to nearest tab stop. */
+
+static unsigned int
+next_tab_stop (unsigned int vis_column)
+{
+  const unsigned int tab_width = cpp_opts->tabstop;
+  vis_column = ((vis_column + tab_width) / tab_width) * tab_width;
+  return vis_column;
+}
+
 /* Convert libcpp's notion of a column (a 1-based char count) to
    the "visual column" (0-based column, respecting tabs), by reading the
    relevant line.
@@ -77,11 +87,7 @@ get_visual_column (expanded_location exploc, location_t loc,
 	}
 
       if (ch == '\t')
-       {
-	 /* Round up to nearest tab stop. */
-	 const unsigned int tab_width = cpp_opts->tabstop;
-	 vis_column = ((vis_column + tab_width) / tab_width) * tab_width;
-       }
+	vis_column = next_tab_stop (vis_column);
       else
        vis_column++;
     }
@@ -93,54 +99,49 @@ get_visual_column (expanded_location exploc, location_t loc,
   return true;
 }
 
-/* Does the given source line appear to contain a #if directive?
-   (or #ifdef/#ifndef).  Ignore the possibility of it being inside a
-   comment, for simplicity.
-   Helper function for detect_preprocessor_logic.  */
+/* Attempt to determine the first non-whitespace character in line LINE_NUM
+   of source line FILE.
+
+   If this is possible, return true and write its "visual column" to
+   *FIRST_NWS.
+   Otherwise, return false, leaving *FIRST_NWS untouched.  */
 
 static bool
-line_contains_hash_if (const char *file, int line_num)
+get_first_nws_vis_column (const char *file, int line_num,
+			  unsigned int *first_nws)
 {
+  gcc_assert (first_nws);
+
   int line_len;
   const char *line = location_get_source_line (file, line_num, &line_len);
   if (!line)
     return false;
+  unsigned int vis_column = 0;
+  for (int i = 1; i < line_len; i++)
+    {
+      unsigned char ch = line[i - 1];
 
-  int idx;
-
-  /* Skip leading whitespace.  */
-  for (idx = 0; idx < line_len; idx++)
-    if (!ISSPACE (line[idx]))
-      break;
-  if (idx == line_len)
-    return false;
-
-  /* Require a '#' character.  */
-  if (line[idx] != '#')
-    return false;
-  idx++;
+      if (!ISSPACE (ch))
+	{
+	  *first_nws = vis_column;
+	  return true;
+	}
 
-  /* Skip whitespace.  */
-  while (idx < line_len)
-    {
-      if (!ISSPACE (line[idx]))
-	break;
-      idx++;
+      if (ch == '\t')
+	vis_column = next_tab_stop (vis_column);
+      else
+	vis_column++;
     }
 
-  /* Match #if/#ifdef/#ifndef.  */
-  if (idx + 2 <= line_len)
-    if (line[idx] == 'i')
-      if (line[idx + 1] == 'f')
-	return true;
-
+  /* No non-whitespace characters found.  */
   return false;
 }
 
-
-/* Determine if there is preprocessor logic between
+/* Determine if there is an unindent/outdent between
    BODY_EXPLOC and NEXT_STMT_EXPLOC, to ensure that we don't
-   issue a warning for cases like this:
+   issue a warning for cases like the following:
+
+   (1) Preprocessor logic
 
 	if (flagA)
 	  foo ();
@@ -151,31 +152,47 @@ line_contains_hash_if (const char *file, int line_num)
 	  bar ();
 	  ^ NEXT_STMT_EXPLOC
 
-   despite "bar ();" being visually aligned below "foo ();" and
-   being (as far as the parser sees) the next token.
+   "bar ();" is visually aligned below "foo ();" and
+   is (as far as the parser sees) the next token, but
+   this isn't misleading to a human reader.
 
-   Return true if such logic is detected.  */
+   (2) Empty macro with bad indentation
 
-static bool
-detect_preprocessor_logic (expanded_location body_exploc,
-			   expanded_location next_stmt_exploc)
-{
-  gcc_assert (next_stmt_exploc.file == body_exploc.file);
-  gcc_assert (next_stmt_exploc.line > body_exploc.line);
+   In the following, the
+     "if (i > 0)"
+   is poorly indented, and ought to be on the same column as
+      "engine_ref_debug(e, 0, -1)"
+   However, it is not misleadingly indented, due to the presence
+   of that macro.
 
-  if (next_stmt_exploc.line - body_exploc.line < 4)
-    return false;
+      #define engine_ref_debug(X, Y, Z)
+
+      if (locked)
+        i = foo (0);
+      else
+        i = foo (1);
+      engine_ref_debug(e, 0, -1)
+        if (i > 0)
+        return 1;
 
-  /* Is there a #if/#ifdef/#ifndef directive somewhere in the lines
-     between the given locations?
+   Return true if such an unindent/outdent is detected.  */
 
-     This is something of a layering violation, but by necessity,
-     given the nature of what we're testing for.  For example,
-     in theory we could be fooled by a #if within a comment, but
-     it's unlikely to matter.  */
-  for (int line = body_exploc.line + 1; line < next_stmt_exploc.line; line++)
-    if (line_contains_hash_if (body_exploc.file, line))
-      return true;
+static bool
+detect_intervening_unindent (const char *file,
+			     int body_line,
+			     int next_stmt_line,
+			     unsigned int vis_column)
+{
+  gcc_assert (file);
+  gcc_assert (next_stmt_line > body_line);
+
+  for (int line = body_line + 1; line < next_stmt_line; line++)
+    {
+      unsigned int line_vis_column;
+      if (get_first_nws_vis_column (file, line, &line_vis_column))
+	if (line_vis_column < vis_column)
+	  return true;
+    }
 
   /* Not found.  */
   return false;
@@ -467,9 +484,11 @@ should_warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 	  if (body_vis_column <= guard_line_first_nws)
 	    return false;
 
-	  /* Don't warn if there is multiline preprocessor logic between
-	     the two statements. */
-	  if (detect_preprocessor_logic (body_exploc, next_stmt_exploc))
+	  /* Don't warn if there is an unindent between the two statements. */
+	  int vis_column = MIN (next_stmt_vis_column, body_vis_column);
+	  if (detect_intervening_unindent (body_exploc.file, body_exploc.line,
+					   next_stmt_exploc.line,
+					   vis_column))
 	    return false;
 
 	  /* Otherwise, they are visually aligned: issue a warning.  */
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
index 04500b7..7b499d4 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
@@ -982,3 +982,75 @@ fn_41_b (void)
   if (!flagC)
     goto fail;
 }
+
+/* In the following, the
+     "if (i > 0)"
+   is poorly indented, and ought to be on the same column as
+      "engine_ref_debug(e, 0, -1)"
+   However, it is not misleadingly indented, due to the presence
+   of that macro.  Verify that we do not emit a warning about it
+   not being guarded by the "else" clause above.
+
+   Based on an example seen in OpenSSL 1.0.1, which was filed as
+   PR c/68187 in comment #1, though it's arguably a separate bug to
+   the one in comment #0.  */
+
+int
+fn_42_a (int locked)
+{
+#define engine_ref_debug(X, Y, Z)
+
+    int i;
+
+    if (locked)
+        i = foo (0);
+    else
+        i = foo (1);
+    engine_ref_debug(e, 0, -1)
+        if (i > 0)
+        return 1;
+    return 0;
+#undef engine_ref_debug
+}
+
+/* As above, but the empty macro is at the same indentation level.
+   This *is* misleading; verify that we do emit a warning about it.  */
+
+int
+fn_42_b (int locked)
+{
+#define engine_ref_debug(X, Y, Z)
+
+    int i;
+
+    if (locked)
+        i = foo (0);
+    else /* { dg-message "...this .else. clause" } */
+        i = foo (1);
+        engine_ref_debug(e, 0, -1)
+        if (i > 0) /* { dg-warning "statement is indented" } */
+        return 1;
+    return 0;
+#undef engine_ref_debug
+}
+
+/* As above, but where the body is a semicolon "hidden" by a preceding
+   comment, where the semicolon is not in the same column as the successor
+   "if" statement, but the empty macro expansion is at the same indentation
+   level as the guard.
+   This is poor indentation, but not misleading; verify that we don't emit a
+   warning about it.  */
+
+int
+fn_42_c (int locked, int i)
+{
+#define engine_ref_debug(X, Y, Z)
+
+    if (locked)
+        /* blah */;
+    engine_ref_debug(e, 0, -1)
+        if (i > 0)
+        return 1;
+    return 0;
+#undef engine_ref_debug
+}
-- 
1.8.5.3

Re: [PATCH 2/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #1)

[Patrick Palka]

On Thu, Mar 3, 2016 at 10:21 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> Comment #1 of PR c/68187 identified another overzealous warning
> from -Wmisleading-indentation, with OpenSSL 1.0.1, on this poorly
> indented code:
>
> 115    if (locked)
> 116        i = CRYPTO_add(&e->struct_ref, -1, CRYPTO_LOCK_ENGINE);
> 117    else
> 118        i = --e->struct_ref;
> 119    engine_ref_debug(e, 0, -1)
> 120        if (i > 0)
> 121        return 1;
>
> eng_lib.c:120:9: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
>          if (i > 0)
>          ^~
> eng_lib.c:117:5: note: ...this 'else' clause, but it is not
>      else
>      ^~~~
>
> Line 120 is poorly indented, but the warning is arguably unjustified.
>
> Root cause is that "engine_ref_debug" is actually a debugging macro
> that was empty in the given configuration, so the code effectively
> was:
>
> 117    else                      // GUARD
> 118        i = --e->struct_ref;  // BODY
> 119
> 120        if (i > 0)            // NEXT
>
> hence the warning.
>
> But the code as seen by a human is clearly *not* misleading, and
> hence arguably we shouldn't warn for this case.
>
> The following patch fixes this by ruling that if there is non-whitespace
> in a line between the BODY and the NEXT statements, and that this
> non-whitespace is effectively an "unindent" or "outdent" (it's not clear
> to me which of these terms is better), then to not issue a warning.

Cool, this also fixes the false-positives seen in bdwgc, whose coding
style suggests indenting things inside an #ifdef as if it were an
if(), e.g.:

    if (a)
      foo ();
#   ifndef A
      bar ();
#   endif
    ...

Re: [PATCH 2/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #1)

[Bernd Schmidt]

On 03/03/2016 06:15 PM, Patrick Palka wrote:
> Cool, this also fixes the false-positives seen in bdwgc, whose coding
> style suggests indenting things inside an #ifdef as if it were an
> if(), e.g.:
>
>      if (a)
>        foo ();
> #   ifndef A
>        bar ();
> #   endif
>      ...

Once again I find myself thinking this is not a false positive, but 
terrible code we should warn for.


Bernd

Re: [PATCH 2/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #1)

[Marek Polacek]

On Fri, Mar 04, 2016 at 01:53:20PM +0100, Bernd Schmidt wrote:
> On 03/03/2016 06:15 PM, Patrick Palka wrote:
> >Cool, this also fixes the false-positives seen in bdwgc, whose coding
> >style suggests indenting things inside an #ifdef as if it were an
> >if(), e.g.:
> >
> >     if (a)
> >       foo ();
> >#   ifndef A
> >       bar ();
> >#   endif
> >     ...
> 
> Once again I find myself thinking this is not a false positive, but terrible
> code we should warn for.

I agree.  It seems entirely sane to warn here.

	Marek

Re: [PATCH 2/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #1)

[Jeff Law]

On 03/03/2016 08:21 AM, David Malcolm wrote:
> Comment #1 of PR c/68187 identified another overzealous warning
> from -Wmisleading-indentation, with OpenSSL 1.0.1, on this poorly
> indented code:
>
> 115    if (locked)
> 116        i = CRYPTO_add(&e->struct_ref, -1, CRYPTO_LOCK_ENGINE);
> 117    else
> 118        i = --e->struct_ref;
> 119    engine_ref_debug(e, 0, -1)
> 120        if (i > 0)
> 121        return 1;
Egad.  How do people read this code when they have to understand it and 
make changes.    What a steaming pile of .....


>
> Root cause is that "engine_ref_debug" is actually a debugging macro
> that was empty in the given configuration, so the code effectively
> was:
>
> 117    else                      // GUARD
> 118        i = --e->struct_ref;  // BODY
> 119
> 120        if (i > 0)            // NEXT
>
> hence the warning.
No surprise we triggered seeing that.

>
> OK for trunk?
>
> Note: one of the new test cases adds a dg-warning/dg-message pair, and so
> would require updating if/when the wording change posted here:
>    https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00068.html
>    ("[PATCH] PR c/69993: improvements to wording of -Wmisleading-indentation")
> is applied.
>
> gcc/c-family/ChangeLog:
> 	PR c/68187
> 	* c-indentation.c (get_visual_column): Move code to determine next
> 	tab stop to...
> 	(next_tab_stop): ...this new function.
> 	(line_contains_hash_if): Delete function.
> 	(detect_preprocessor_logic): Delete function.
> 	(get_first_nws_vis_column): New function.
> 	(detect_intervening_unindent): New function.
> 	(should_warn_for_misleading_indentation): Replace call to
> 	detect_preprocessor_logic with a call to
> 	detect_intervening_unindent.
>
> gcc/testsuite/ChangeLog:
> 	PR c/68187
> 	* c-c++-common/Wmisleading-indentation.c (fn_42_a): New test
> 	function.
> 	(fn_42_b): Likewise.
> 	(fn_42_c): Likewise.
OK.


Jeff

Re: [PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0)

[Patrick Palka]

On Thu, Mar 3, 2016 at 10:21 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> PR c/68187 covers two cases involving poor indentation where
> the indentation is arguably not misleading, but for which
> -Wmisleading-indentation emits a warning.
>
> The two cases appear to be different in nature; one in comment #0
> and the other in comment #1.  Richi marked the bug as a whole as
> a P1 regression; it's not clear to me if he meant one or both of
> these cases, so the following two patches fix both.
>
> The rest of this post addresses the case in comment #0 of the PR;
> the followup post addresses the other case, in comment #1 of the PR.
>
> Building glibc (a9224562cbe9cfb0bd8d9e637a06141141f9e6e3) on x86_64
> led to this diagnostic from -Wmisleading-indentation:
>
> ../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)
>          ^
>
> 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;
>
> Lines 354 and 355 are poorly indented, leading to the warning from
> -Wmisleading-indentation at line 356.
>
> The wording of the warning is clearly wrong: line 356 isn't indented as if
> guarded by line 353, it's more that lines 354 and 355 *aren't* indented.
>
> What's happening is that should_warn_for_misleading_indentation has a
> heuristic for handling "} else", such as:
>
>      if (p)
>        foo (1);
>      } else       // GUARD
>        foo (2);   // BODY
>        foo (3);   // NEXT
>
> and this heuristic uses the first non-whitespace character in the line
> containing GUARD as the column of interest: the "}" character.
>
> In this case we have:
>
>    353        && ({ for (cnt = 0; cnt < thousands_len; ++cnt)  // GUARD
>    354              if (thousands[cnt] != end[cnt])            // BODY
>    355                break;
>    356              cnt < thousands_len; })                    // NEXT
>
> and so it uses the column of the "&&", and treats it as if it were
> indented thus:
>
>    353        for (cnt = 0; cnt < thousands_len; ++cnt)        // GUARD
>    354              if (thousands[cnt] != end[cnt])            // BODY
>    355                break;
>    356              cnt < thousands_len; })                    // NEXT
>
> and thus issues a warning.
>
> As far as I can tell the heuristic in question only makes sense for
> "else" clauses, so the following patch updates it to only use the
> special column when handling "else" clauses, eliminating the
> overzealous warning.

Wouldn't this change have the undesirable effect of no longer warning about:

      if (p)
        foo (1);
      } else if (q)
        foo (2);
        foo (3);

Re: [PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0)

[David Malcolm]

[-- Attachment #1: Type: text/plain, Size: 4844 bytes --]

On Thu, 2016-03-03 at 10:24 -0500, Patrick Palka wrote:
> On Thu, Mar 3, 2016 at 10:21 AM, David Malcolm <dmalcolm@redhat.com>
> wrote:
> > PR c/68187 covers two cases involving poor indentation where
> > the indentation is arguably not misleading, but for which
> > -Wmisleading-indentation emits a warning.
> > 
> > The two cases appear to be different in nature; one in comment #0
> > and the other in comment #1.  Richi marked the bug as a whole as
> > a P1 regression; it's not clear to me if he meant one or both of
> > these cases, so the following two patches fix both.
> > 
> > The rest of this post addresses the case in comment #0 of the PR;
> > the followup post addresses the other case, in comment #1 of the
> > PR.
> > 
> > Building glibc (a9224562cbe9cfb0bd8d9e637a06141141f9e6e3) on x86_64
> > led to this diagnostic from -Wmisleading-indentation:
> > 
> > ../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)
> >          ^
> > 
> > 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;
> > 
> > Lines 354 and 355 are poorly indented, leading to the warning from
> > -Wmisleading-indentation at line 356.
> > 
> > The wording of the warning is clearly wrong: line 356 isn't
> > indented as if
> > guarded by line 353, it's more that lines 354 and 355 *aren't*
> > indented.
> > 
> > What's happening is that should_warn_for_misleading_indentation has
> > a
> > heuristic for handling "} else", such as:
> > 
> >      if (p)
> >        foo (1);
> >      } else       // GUARD
> >        foo (2);   // BODY
> >        foo (3);   // NEXT
> > 
> > and this heuristic uses the first non-whitespace character in the
> > line
> > containing GUARD as the column of interest: the "}" character.
> > 
> > In this case we have:
> > 
> >    353        && ({ for (cnt = 0; cnt < thousands_len; ++cnt)  //
> > GUARD
> >    354              if (thousands[cnt] != end[cnt])            //
> > BODY
> >    355                break;
> >    356              cnt < thousands_len; })                    //
> > NEXT
> > 
> > and so it uses the column of the "&&", and treats it as if it were
> > indented thus:
> > 
> >    353        for (cnt = 0; cnt < thousands_len; ++cnt)        //
> > GUARD
> >    354              if (thousands[cnt] != end[cnt])            //
> > BODY
> >    355                break;
> >    356              cnt < thousands_len; })                    //
> > NEXT
> > 
> > and thus issues a warning.
> > 
> > As far as I can tell the heuristic in question only makes sense for
> > "else" clauses, so the following patch updates it to only use the
> > special column when handling "else" clauses, eliminating the
> > overzealous warning.
> 
> Wouldn't this change have the undesirable effect of no longer warning
> about:
> 
>       if (p)
>         foo (1);
>       } else if (q)
>         foo (2);
>         foo (3);

No, because the rejection based on indentation is done relative to
 guard_line_first_nws, rather than guard_vis_column (I tried doing it
via the latter in one version of the patch, and that broke some of the
existing cases, so I didn't make that change).

See the attached test file (which doesn't have dg-directives yet); the
example you give is test1_d, with an open-brace added to the "if (p)".

Trunk emits warnings for:
  * test1_c
  * test1_d
  * test1_e
  * test1_f (two warnings, one for the "if", one for the "else")
  * test1_g
  * test2_c
  * test2_d
  * test2_e

With the patches, it emits warnings for:
  * test1_c
  * test1_d
  * test1_e
  * test1_f (just one warnings, for the "if")
  * test1_g
  * test2_c
  * test2_d
  * test2_e

so the only change is the removal of the erroneous double warning for
the "else" in test1_f.

I can add dg-directives and add the attachment to Wmisleading
-indentation.c as part of the patch (or keep it as a separate new test
file, the former is getting large).

[-- Attachment #2: Wmisleading-indentation-4.c --]
[-- Type: text/x-csrc, Size: 1830 bytes --]

/* PR c/68187.  */
/* { dg-options "-Wmisleading-indentation" } */
/* { dg-do compile } */

extern int foo (int);
extern int bar (int, int);
extern int flagA;
extern int flagB;
extern int flagC;
extern int flagD;

/* Before the "}".  */

void
test1_a (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
 foo (2);
 foo (3);
}

/* Aligned with the "}".  */

void
test1_b (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
  foo (2);
  foo (3);
}

/* Indented between the "}" and the "else".  */

void
test1_c (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
   foo (2);
   foo (3);
}

/* Aligned with the "else".  */

void
test1_d (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
    foo (2);
    foo (3);
}

/* Indented between the "else" and the "if".  */

void
test1_e (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
      foo (2);
      foo (3);
}

/* Aligned with the "if".  */

void
test1_f (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
         foo (2);
         foo (3);
}

/* Indented more than the "if".  */

void
test1_g (void)
{
  if (flagA) {
    foo (1);
  } else if (flagB)
            foo (2);
            foo (3);
}

/* Again, but without the 2nd "if".  */

/* Before the "}".  */

void
test2_a (void)
{
  if (flagA) {
    foo (1);
  } else
 foo (2);
 foo (3);
}

/* Aligned with the "}".  */

void
test2_b (void)
{
  if (flagA) {
    foo (1);
  } else
  foo (2);
  foo (3);
}

/* Indented between the "}" and the "else".  */

void
test2_c (void)
{
  if (flagA) {
    foo (1);
  } else
   foo (2);
   foo (3);
}

/* Aligned with the "else".  */

void
test2_d (void)
{
  if (flagA) {
    foo (1);
  } else
    foo (2);
    foo (3);
}

/* Indented more than the "else".  */

void
test2_e (void)
{
  if (flagA) {
    foo (1);
  } else
        foo (2);
        foo (3);
}

Re: [PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0)

[Patrick Palka]

On Thu, Mar 3, 2016 at 10:56 AM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Thu, 2016-03-03 at 10:24 -0500, Patrick Palka wrote:
>> On Thu, Mar 3, 2016 at 10:21 AM, David Malcolm <dmalcolm@redhat.com>
>> wrote:
>> > PR c/68187 covers two cases involving poor indentation where
>> > the indentation is arguably not misleading, but for which
>> > -Wmisleading-indentation emits a warning.
>> >
>> > The two cases appear to be different in nature; one in comment #0
>> > and the other in comment #1.  Richi marked the bug as a whole as
>> > a P1 regression; it's not clear to me if he meant one or both of
>> > these cases, so the following two patches fix both.
>> >
>> > The rest of this post addresses the case in comment #0 of the PR;
>> > the followup post addresses the other case, in comment #1 of the
>> > PR.
>> >
>> > Building glibc (a9224562cbe9cfb0bd8d9e637a06141141f9e6e3) on x86_64
>> > led to this diagnostic from -Wmisleading-indentation:
>> >
>> > ../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)
>> >          ^
>> >
>> > 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;
>> >
>> > Lines 354 and 355 are poorly indented, leading to the warning from
>> > -Wmisleading-indentation at line 356.
>> >
>> > The wording of the warning is clearly wrong: line 356 isn't
>> > indented as if
>> > guarded by line 353, it's more that lines 354 and 355 *aren't*
>> > indented.
>> >
>> > What's happening is that should_warn_for_misleading_indentation has
>> > a
>> > heuristic for handling "} else", such as:
>> >
>> >      if (p)
>> >        foo (1);
>> >      } else       // GUARD
>> >        foo (2);   // BODY
>> >        foo (3);   // NEXT
>> >
>> > and this heuristic uses the first non-whitespace character in the
>> > line
>> > containing GUARD as the column of interest: the "}" character.
>> >
>> > In this case we have:
>> >
>> >    353        && ({ for (cnt = 0; cnt < thousands_len; ++cnt)  //
>> > GUARD
>> >    354              if (thousands[cnt] != end[cnt])            //
>> > BODY
>> >    355                break;
>> >    356              cnt < thousands_len; })                    //
>> > NEXT
>> >
>> > and so it uses the column of the "&&", and treats it as if it were
>> > indented thus:
>> >
>> >    353        for (cnt = 0; cnt < thousands_len; ++cnt)        //
>> > GUARD
>> >    354              if (thousands[cnt] != end[cnt])            //
>> > BODY
>> >    355                break;
>> >    356              cnt < thousands_len; })                    //
>> > NEXT
>> >
>> > and thus issues a warning.
>> >
>> > As far as I can tell the heuristic in question only makes sense for
>> > "else" clauses, so the following patch updates it to only use the
>> > special column when handling "else" clauses, eliminating the
>> > overzealous warning.
>>
>> Wouldn't this change have the undesirable effect of no longer warning
>> about:
>>
>>       if (p)
>>         foo (1);
>>       } else if (q)
>>         foo (2);
>>         foo (3);
>
> No, because the rejection based on indentation is done relative to
>  guard_line_first_nws, rather than guard_vis_column (I tried doing it
> via the latter in one version of the patch, and that broke some of the
> existing cases, so I didn't make that change).

I see. That clears things up for me, thanks.

>
> See the attached test file (which doesn't have dg-directives yet); the
> example you give is test1_d, with an open-brace added to the "if (p)".
>
> Trunk emits warnings for:
>   * test1_c
>   * test1_d
>   * test1_e
>   * test1_f (two warnings, one for the "if", one for the "else")
>   * test1_g
>   * test2_c
>   * test2_d
>   * test2_e
>
> With the patches, it emits warnings for:
>   * test1_c
>   * test1_d
>   * test1_e
>   * test1_f (just one warnings, for the "if")
>   * test1_g
>   * test2_c
>   * test2_d
>   * test2_e
>
> so the only change is the removal of the erroneous double warning for
> the "else" in test1_f.

Nice!

[committed 1/2] Wmisleading-indentation: add reproducer for PR c/70085

[David Malcolm]

PR c/70085 reported a false-positive from -Wmisleading-indentation.

The warning was fixed by the fix for PR c/68187 (r233972), but it seems
worth capturing the reproducer for PR c/70085 as an additional test case,
as it's slightly different to those seen in PR c/68187.

Committed to trunk (as "obvious") as r234145.

gcc/testsuite/ChangeLog:
	PR c/70085
	* c-c++-common/Wmisleading-indentation.c (pr70085): New test case.
---
 gcc/testsuite/c-c++-common/Wmisleading-indentation.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
index 7b499d4..38c8aec 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
@@ -1054,3 +1054,19 @@ fn_42_c (int locked, int i)
     return 0;
 #undef engine_ref_debug
 }
+
+/* We shouldn't complain about the following function.  */
+#define ENABLE_FEATURE
+int pr70085 (int x, int y)
+{
+  if (x > y)
+    return x - y;
+
+  #ifdef ENABLE_FEATURE
+    if (x == y)
+      return 0;
+  #endif
+
+  return -1;
+}
+#undef ENABLE_FEATURE
-- 
1.8.5.3

[committed 2/2] Wmisleading-indentation.c: add more test cases for PR c/68187

[David Malcolm]

I posted a series of tests here:
  https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00271.html
as part of the discussion around PR c/68187.

I've cleaned them into DejaGnu form and added them to the existing
test file; they add 16 PASS results to gcc.sum and 48 PASS results
to g++.sum.

Committed to trunk as r234146 (as "obvious").

gcc/testsuite/ChangeLog:
	PR c/68187
	* c-c++-common/Wmisleading-indentation.c (test43_a): New test
	case.
	(test43_b): Likewise.
	(test43_c): Likewise.
	(test43_d): Likewise.
	(test43_e): Likewise.
	(test43_f): Likewise.
	(test43_g): Likewise.
	(test44_a): Likewise.
	(test44_b): Likewise.
	(test44_c): Likewise.
	(test44_d): Likewise.
	(test44_e): Likewise.
---
 .../c-c++-common/Wmisleading-indentation.c         | 168 +++++++++++++++++++++
 1 file changed, 168 insertions(+)

diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
index 38c8aec..ba512e7 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
@@ -1070,3 +1070,171 @@ int pr70085 (int x, int y)
   return -1;
 }
 #undef ENABLE_FEATURE
+
+/* Additional test coverage for PR c/68187, with various locations for a
+   pair of aligned statements ("foo (2);" and "foo (3);") that may or may
+   not be misleadingly indented.  */
+
+/* Before the "}".
+
+   The two statements aren't visually "within" the above line, so we
+   shouldn't warn.  */
+
+void
+test43_a (void)
+{
+  if (flagA) {
+    foo (1);
+  } else if (flagB)
+ foo (2);
+ foo (3);
+}
+
+/* Aligned with the "}".
+
+   Again, the two statements aren't visually "within" the above line, so we
+   shouldn't warn.  */
+
+void
+test43_b (void)
+{
+  if (flagA) {
+    foo (1);
+  } else if (flagB)
+  foo (2);
+  foo (3);
+}
+
+/* Indented between the "}" and the "else".
+
+   The two statements are indented "within" the line above, so appear that
+   they would be guarded together.  We should warn about this.  */
+
+void
+test43_c (void)
+{
+  if (flagA) {
+    foo (1);
+  } else if (flagB) /* { dg-message "...this .if. clause" } */
+   foo (2);
+   foo (3); /* { dg-warning "statement is indented" } */
+}
+
+/* Aligned with the "else".  Likewise, we should warn.  */
+
+void
+test43_d (void)
+{
+  if (flagA) {
+    foo (1);
+  } else if (flagB) /* { dg-message "...this .if. clause" } */
+    foo (2);
+    foo (3); /* { dg-warning "statement is indented" } */
+}
+
+/* Indented between the "else" and the "if".  Likewise, we should warn.  */
+
+void
+test43_e (void)
+{
+  if (flagA) {
+    foo (1);
+  } else if (flagB) /* { dg-message "...this .if. clause" } */
+      foo (2);
+      foo (3); /* { dg-warning "statement is indented" } */
+}
+
+/* Aligned with the "if".  Likewise, we should warn.  */
+
+void
+test43_f (void)
+{
+  if (flagA) {
+    foo (1);
+  } else if (flagB) /* { dg-message "...this .else. clause" } */
+         foo (2);
+         foo (3); /* { dg-warning "statement is indented" } */
+}
+
+/* Indented more than the "if".  Likewise, we should warn.  */
+
+void
+test43_g (void)
+{
+  if (flagA) {
+    foo (1);
+  } else if (flagB) /* { dg-message "...this .if. clause" } */
+            foo (2);
+            foo (3); /* { dg-warning "statement is indented" } */
+}
+
+/* Again, but without the 2nd "if".  */
+
+/* Before the "}".
+
+   As before, the two statements aren't visually "within" the above line,
+   so we shouldn't warn.  */
+
+void
+test44_a (void)
+{
+  if (flagA) {
+    foo (1);
+  } else
+ foo (2);
+ foo (3);
+}
+
+/* Aligned with the "}".
+
+   As before, the two statements aren't visually "within" the above line,
+   so we shouldn't warn.  */
+
+void
+test44_b (void)
+{
+  if (flagA) {
+    foo (1);
+  } else
+  foo (2);
+  foo (3);
+}
+
+/* Indented between the "}" and the "else".
+
+   The two statements are indented "within" the line above, so appear that
+   they would be guarded together.  We should warn about this.  */
+
+void
+test44_c (void)
+{
+  if (flagA) {
+    foo (1);
+  } else  /* { dg-message "...this .else. clause" } */
+   foo (2);
+   foo (3);  /* { dg-warning "statement is indented" } */
+}
+
+/* Aligned with the "else".  Likewise, we should warn.  */
+
+void
+test44_d (void)
+{
+  if (flagA) {
+    foo (1);
+  } else  /* { dg-message "...this .else. clause" } */
+    foo (2);
+    foo (3);  /* { dg-warning "statement is indented" } */
+}
+
+/* Indented more than the "else".  Likewise, we should warn.  */
+
+void
+test44_e (void)
+{
+  if (flagA) {
+    foo (1);
+  } else  /* { dg-message "...this .else. clause" } */
+        foo (2);
+        foo (3);  /* { dg-warning "statement is indented" } */
+}
-- 
1.8.5.3

Re: [PATCH 1/2] PR c/68187: fix overzealous -Wmisleading-indentation (comment #0)

[Jeff Law]

On 03/03/2016 08:21 AM, David Malcolm wrote:
> PR c/68187 covers two cases involving poor indentation where
> the indentation is arguably not misleading, but for which
> -Wmisleading-indentation emits a warning.
>
> The two cases appear to be different in nature; one in comment #0
> and the other in comment #1.  Richi marked the bug as a whole as
> a P1 regression; it's not clear to me if he meant one or both of
> these cases, so the following two patches fix both.
>
> The rest of this post addresses the case in comment #0 of the PR;
> the followup post addresses the other case, in comment #1 of the PR.
>
> Building glibc (a9224562cbe9cfb0bd8d9e637a06141141f9e6e3) on x86_64
> led to this diagnostic from -Wmisleading-indentation:
>
> ../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)
>           ^
>
> 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;
>
> Lines 354 and 355 are poorly indented, leading to the warning from
> -Wmisleading-indentation at line 356.
>
> The wording of the warning is clearly wrong: line 356 isn't indented as if
> guarded by line 353, it's more that lines 354 and 355 *aren't* indented.
>
> What's happening is that should_warn_for_misleading_indentation has a
> heuristic for handling "} else", such as:
>
>       if (p)
>         foo (1);
>       } else       // GUARD
>         foo (2);   // BODY
>         foo (3);   // NEXT
>
> and this heuristic uses the first non-whitespace character in the line
> containing GUARD as the column of interest: the "}" character.
>
> In this case we have:
>
>     353        && ({ for (cnt = 0; cnt < thousands_len; ++cnt)  // GUARD
>     354              if (thousands[cnt] != end[cnt])            // BODY
>     355                break;
>     356              cnt < thousands_len; })                    // NEXT
>
> and so it uses the column of the "&&", and treats it as if it were
> indented thus:
>
>     353        for (cnt = 0; cnt < thousands_len; ++cnt)        // GUARD
>     354              if (thousands[cnt] != end[cnt])            // BODY
>     355                break;
>     356              cnt < thousands_len; })                    // NEXT
>
> and thus issues a warning.
>
> As far as I can tell the heuristic in question only makes sense for
> "else" clauses, so the following patch updates it to only use the
> special column when handling "else" clauses, eliminating the
> overzealous warning.
>
> Doing so led to a nonsensical warning for
> libstdc++-v3/src/c++11/random.cc:random_device::_M_init:
>
> random.cc: In member function ‘void std::random_device::_M_init(const string&)’:
> random.cc:102:10: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
>       else if (token != "/dev/urandom" && token != "/dev/random")
>            ^~
> random.cc:107:5: note: ...this statement, but the latter is indented as if it does
>       _M_file = static_cast<void*>(std::fopen(fname, "rb"));
>       ^~~~~~~
>
> so the patch addresses this by tweaking the heuristic that rejects
> aligned BODY and NEXT so that it doesn't require them to be aligned with
> the first non-whitespace of the GUARD, simply that they not be indented
> relative to it.
>
> Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu in
> combination with the following patch; standalone bootstrap&regrtest
> is in progress.
>
> OK for trunk if the latter is successful?
>
> gcc/c-family/ChangeLog:
> 	PR c/68187
> 	* c-indentation.c (should_warn_for_misleading_indentation): When
> 	suppressing warnings about cases where the guard and body are on
> 	the same column, only use the first non-whitespace column in place
> 	of the guard token column when dealing with "else" clauses.
> 	When rejecting aligned BODY and NEXT, loosen the requirement
> 	from equality with the first non-whitespace of guard to simply
> 	that they not be indented relative to it.
>
> gcc/testsuite/ChangeLog:
> 	PR c/68187
> 	* c-c++-common/Wmisleading-indentation.c (fn_40_a): New test
> 	function.
> 	(fn_40_b): Likewise.
> 	(fn_41_a): Likewise.
> 	(fn_41_b): Likewise.
OK.

FWIW, I think all of c-indentation would fall under the diagnostics 
maintainership you picked up recently.

jeff