diff mbox

PR c/66220: Fix false positive from -Wmisleading-indentation

Message ID 1432141551-13605-1-git-send-email-dmalcolm@redhat.com
State New
Headers show

Commit Message

David Malcolm May 20, 2015, 5:05 p.m. UTC 
This patch fixes the false positive seen from -Wmisleading-indentation
on this code:

    if (v == 2)
    {
        res = 27;
    } else
    {
        res = 18;
    }
    return res;
    ^ FALSE POSITIVE HERE

along with similar code seen when I tested it with linux-4.0.3.

The patch adds a reject for the case where the guard ("else" in
the above example) is more indented than the things it guards.

Doing so uncovered an issue with this testcase:

#define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-message "36: ...this 'for' clause, but it is not" } */
void fn_15 (void)
{
  int i;
  FOR_EACH (i, 0, 10) /* { dg-message "3: in expansion of macro" } */
    foo (i);
    bar (i, i); /* { dg-warning "statement is indented as if it were guarded by..." } */
}
#undef FOR_EACH

which would then fail to report the warning, due to it using the
location of the "for" in the definition of macro FOR_EACH, rather than
the location of the FOR_EACH (i, 0, 10).  The fix for this is to use
expand_location to get file/line/col of each thing, rather than
expand_location_to_spelling_point.

With that, all testcases in Wmisleading-indentation.c pass (including
the new ones posted in
https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01846.html ).

OK for trunk if it passes bootstrap&regrest?  (only tested with
  make check-gcc RUNTESTFLAGS="dg.exp=Wmisleading-indentation.c"
  make check-g++ RUNTESTFLAGS="dg.exp=Wmisleading-indentation.c"
so far)

gcc/c-family/ChangeLog:
	PR c/66220:
	* c-indentation.c (should_warn_for_misleading_indentation): Use
	expand_location rather than expand_location_to_spelling_point.
	Don't warn if the guarding statement is more indented than the
	next/body stmts.

gcc/testsuite/ChangeLog:
	PR c/66220:
	* c-c++-common/Wmisleading-indentation.c (fn_35): New.
	(fn_36): New.
---
 gcc/c-family/c-indentation.c                       | 26 ++++++++++-----
 .../c-c++-common/Wmisleading-indentation.c         | 38 ++++++++++++++++++++++
 2 files changed, 56 insertions(+), 8 deletions(-)

Comments

Jeff Law May 20, 2015, 8:33 p.m. UTC | #1 
On 05/20/2015 11:05 AM, David Malcolm wrote:
> This patch fixes the false positive seen from -Wmisleading-indentation
> on this code:
>
>      if (v == 2)
>      {
>          res = 27;
>      } else
>      {
>          res = 18;
>      }
>      return res;
>      ^ FALSE POSITIVE HERE
>
> along with similar code seen when I tested it with linux-4.0.3.
>
> The patch adds a reject for the case where the guard ("else" in
> the above example) is more indented than the things it guards.
>
> Doing so uncovered an issue with this testcase:
>
> #define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-message "36: ...this 'for' clause, but it is not" } */
> void fn_15 (void)
> {
>    int i;
>    FOR_EACH (i, 0, 10) /* { dg-message "3: in expansion of macro" } */
>      foo (i);
>      bar (i, i); /* { dg-warning "statement is indented as if it were guarded by..." } */
> }
> #undef FOR_EACH
>
> which would then fail to report the warning, due to it using the
> location of the "for" in the definition of macro FOR_EACH, rather than
> the location of the FOR_EACH (i, 0, 10).  The fix for this is to use
> expand_location to get file/line/col of each thing, rather than
> expand_location_to_spelling_point.
>
> With that, all testcases in Wmisleading-indentation.c pass (including
> the new ones posted in
> https://gcc.gnu.org/ml/gcc-patches/2015-05/msg01846.html ).
>
> OK for trunk if it passes bootstrap&regrest?  (only tested with
>    make check-gcc RUNTESTFLAGS="dg.exp=Wmisleading-indentation.c"
>    make check-g++ RUNTESTFLAGS="dg.exp=Wmisleading-indentation.c"
> so far)
>
> gcc/c-family/ChangeLog:
> 	PR c/66220:
> 	* c-indentation.c (should_warn_for_misleading_indentation): Use
> 	expand_location rather than expand_location_to_spelling_point.
> 	Don't warn if the guarding statement is more indented than the
> 	next/body stmts.
>
> gcc/testsuite/ChangeLog:
> 	PR c/66220:
> 	* c-c++-common/Wmisleading-indentation.c (fn_35): New.
> 	(fn_36): New.
OK.
jeff
diff mbox

Patch

diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index 9aeebae..1e3a6d8 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -230,10 +230,8 @@  should_warn_for_misleading_indentation (location_t guard_loc,
   if (next_tok_type == CPP_SEMICOLON)
     return false;
 
-  expanded_location body_exploc
-    = expand_location_to_spelling_point (body_loc);
-  expanded_location next_stmt_exploc
-    = expand_location_to_spelling_point (next_stmt_loc);
+  expanded_location body_exploc = expand_location (body_loc);
+  expanded_location next_stmt_exploc = expand_location (next_stmt_loc);
 
   /* They must be in the same file.  */
   if (next_stmt_exploc.file != body_exploc.file)
@@ -257,8 +255,7 @@  should_warn_for_misleading_indentation (location_t guard_loc,
                                           ^ DON'T WARN HERE.  */
   if (next_stmt_exploc.line == body_exploc.line)
     {
-      expanded_location guard_exploc
-	= expand_location_to_spelling_point (guard_loc);
+      expanded_location guard_exploc = expand_location (guard_loc);
       if (guard_exploc.file != body_exploc.file)
 	return true;
       if (guard_exploc.line < body_exploc.line)
@@ -299,6 +296,15 @@  should_warn_for_misleading_indentation (location_t guard_loc,
       #endif
 	  bar ();
 	  ^ DON'T WARN HERE
+
+        if (flag) {
+          foo ();
+        } else
+        {
+          bar ();
+        }
+        baz ();
+        ^ DON'T WARN HERE
   */
   if (next_stmt_exploc.line > body_exploc.line)
     {
@@ -319,14 +325,18 @@  should_warn_for_misleading_indentation (location_t guard_loc,
 	  /* Don't warn if they aren't aligned on the same column
 	     as the guard itself (suggesting autogenerated code that
 	     doesn't bother indenting at all).  */
-	  expanded_location guard_exploc
-	    = expand_location_to_spelling_point (guard_loc);
+	  expanded_location guard_exploc = expand_location (guard_loc);
 	  unsigned int guard_vis_column;
 	  if (!get_visual_column (guard_exploc, &guard_vis_column))
 	    return false;
 	  if (guard_vis_column == body_vis_column)
 	    return false;
 
+	  /* PR 66220: Don't warn if the guarding statement is more
+	     indented than the next/body stmts.  */
+	  if (guard_vis_column > body_vis_column)
+	    return false;
+
 	  /* Don't warn if there is multiline preprocessor logic between
 	     the two statements. */
 	  if (detect_preprocessor_logic (body_exploc, next_stmt_exploc))
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
index 6363d71..443e3dd 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
@@ -653,3 +653,41 @@  void fn_34_indent_linux_style(void)
 		}
 	foo(5);
 }
+
+/* PR 66220.  */
+int fn_35 (int v)
+{
+    int res = 28;
+
+    if (v == 2)
+    {
+        res = 27;
+    } else
+    {
+        res = 18;
+    }
+    return res;
+}
+
+/* This variant of K&R-style formatting (in the presence of conditional
+   compilation) shouldn't lead to a warning.
+
+   Based on false positive seen with r223098 when compiling
+   linux-4.0.3:arch/x86/crypto/aesni-intel_glue.c:aesni_init.  */
+void
+fn_36 (void)
+{
+#if 1 /* e.g. some configuration variable.  */
+	if (flagA) {
+		foo(0);
+		foo(1);
+		foo(2);
+	} else
+#endif
+	{
+		foo(3);
+		foo(4);
+		foo(5);
+	}
+	foo(6); /* We shouldn't warn here.  */
+}