[PATCH] PR c/69993: improvements to wording of -Wmisleading-indentation

[PATCH] PR c/69993: improvements to wording of -Wmisleading-indentation

[David Malcolm]

The wording of our output from -Wmisleading-indentation is rather
confusing, as noted by Reddit user "sysop073" here:
 https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmisleadingindentation_vs_goto_fail/d0eonwd

> The way they split up the warning looks designed to trick you.
> sslKeyExchange.c:631:8: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
>         goto fail;
>         ^~~~
> sslKeyExchange.c:629:4: note: ...this 'if' clause, but it is not
>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>     ^~
> You read the first half and it sounds like goto fail; is guarding something. Why would it not be:
> sslKeyExchange.c:631:8: warning: statement is wrongly indented... [-Wmisleading-indentation]
>         goto fail;
>         ^~~~
> sslKeyExchange.c:629:4: note: ...as if it were guarded by this 'if' clause
>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>     ^~

I agree that the current wording is suboptimal; certainly the wording
would be much clearer if the wording of the "warning" only spoke about the
statement in question, and the "note"/inform should then talk about the
not-really-guarding guard.

One rewording could be:

sslKeyExchange.c:631:8: warning: statement is misleadingly indented... [-Wmisleading-indentation]
        goto fail;
        ^~~~
sslKeyExchange.c:629:4: note: ...as if it were guarded by this 'if' clause, but it is not
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    ^~

However another Reddit user ("ksion") noted here:
  https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmisleadingindentation_vs_goto_fail/d0eqyih
that:
> This is just passive voice, there is nothing tricky about it.
> What I find more confusing -- and what your fix preserves -- is the
> reversed order of offending lines of code in the source file and the message.
>
> I'd rather go with something like this:
> sslKeyExchange.c:629:4: warning: indentation of a statement below this 'if' clause... [-Wmisleading-indentation]
>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>     ^~
> sslKeyExchange.c:631:8: note: ...suggests it is guarded by the 'if' clause, but it's not
>         goto fail;
>         ^~~~
> You can even see how the indentation is wrong in the very error message.

which suggests reversing the order of the messages, so that they appear
in "source" order.

I think this is a big improvement in the readability of the warning.

The attached patch implements such a change, so that the warning is
issued on the supposed guard clause, followed by the note on the
statement that isn't really guarded.

Some examples:

Wmisleading-indentation-3.c:18:3: warning: this 'for' clause does not guard... [-Wmisleading-indentation]
   for (i = 0; i < 10; i++)
   ^~~
Wmisleading-indentation-3.c:20:5: note: ...this statement, but the latter is indented as if it does
     prod[i] = a[i] * b[i];
     ^~~~
Wmisleading-indentation-3.c: In function 'fn_6':
Wmisleading-indentation-3.c:39:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
  if ((err = foo (b)) != 0)
  ^~
Wmisleading-indentation-3.c:41:3: note: ...this statement, but the latter is indented as if it does
   goto fail;
   ^~~~

I'm not totally convinced by my new wording; maybe the note could
also mention the kind of clause ('if'/'while'/'else'/'for') for
clarity, maybe something like:

Wmisleading-indentation-3.c: In function 'fn_6':
Wmisleading-indentation-3.c:39:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
  if ((err = foo (b)) != 0)
  ^~
Wmisleading-indentation-3.c:41:3: note: ...this statement, but the latter is misleadingly indented
as if it is guarded by the 'if'
   goto fail;
   ^~~~

Also, it's slightly clunkier when it comes to macros, e.g.:

Wmisleading-indentation-3.c: In function 'fn_14':
Wmisleading-indentation-3.c:60:3: warning: this 'for' clause does not guard... [-Wmisleading-indentation]
   for ((VAR) = (START); (VAR) < (STOP); (VAR++))
   ^
Wmisleading-indentation-3.c:65:3: note: in expansion of macro 'FOR_EACH'
   FOR_EACH (i, 0, 10)
   ^~~~~~~~
Wmisleading-indentation-3.c:67:5: note: ...this statement, but the latter is indented as if it does
     bar (i, i);
     ^~~

That said, the reordering idea is something I'd like to do for GCC 6.
Failing that, there's the tweak to the wording suggested at the top.

OK for trunk? (assuming we can agree on the wording, and that the latest
version passes testing)

gcc/c-family/ChangeLog:
	PR c/69993
	* c-indentation.c (warn_for_misleading_indentation): Rewrite the
	diagnostic text, reversing the order of the warning and note so
	that they appear in source order.

gcc/testsuite/ChangeLog:
	PR c/69993
	* c-c++-common/Wmisleading-indentation-3.c: New test, based on
	Wmisleading-indentation.c.
	* c-c++-common/Wmisleading-indentation.c: Update thoughout to
	reflect change to diagnostic text and order of messages.
	* gcc.dg/plugin/location-overflow-test-2.c: Likewise.
---
 gcc/c-family/c-indentation.c                       |  10 +-
 .../c-c++-common/Wmisleading-indentation-3.c       |  82 ++++++++++
 .../c-c++-common/Wmisleading-indentation.c         | 166 ++++++++++-----------
 .../gcc.dg/plugin/location-overflow-test-2.c       |   2 +-
 4 files changed, 171 insertions(+), 89 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c

diff --git a/gcc/c-family/c-indentation.c b/gcc/c-family/c-indentation.c
index 521f992..1325567 100644
--- a/gcc/c-family/c-indentation.c
+++ b/gcc/c-family/c-indentation.c
@@ -579,10 +579,10 @@ warn_for_misleading_indentation (const token_indent_info &guard_tinfo,
 					      body_tinfo,
 					      next_tinfo))
     {
-      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",
-		guard_tinfo_to_string (guard_tinfo));
+      if (warning_at (guard_tinfo.location, OPT_Wmisleading_indentation,
+		      "this %qs clause does not guard...",
+		      guard_tinfo_to_string (guard_tinfo)))
+        inform (next_tinfo.location,
+		"...this statement, but the latter is indented as if it does");
     }
 }
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
new file mode 100644
index 0000000..7c14658
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
@@ -0,0 +1,82 @@
+/* Verify -Wmisleading-indentation with source-printing.
+   This is a subset of Wmisleading-indentation.c.  */
+
+/* { dg-options "-Wmisleading-indentation -fdiagnostics-show-caret" } */
+/* { dg-do compile } */
+
+extern int foo (int);
+extern int bar (int, int);
+extern int flagA;
+extern int flagB;
+extern int flagC;
+extern int flagD;
+
+void
+fn_5 (double *a, double *b, double *sum, double *prod)
+{
+  int i = 0;
+  for (i = 0; i < 10; i++) /* { dg-warning "3: this 'for' clause does not guard..." } */
+    sum[i] = a[i] * b[i];
+    prod[i] = a[i] * b[i]; /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
+/* { dg-begin-multiline-output "" }
+   for (i = 0; i < 10; i++)
+   ^~~
+   { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+     prod[i] = a[i] * b[i];
+     ^~~~
+   { dg-end-multiline-output "" } */
+}
+
+/* Based on CVE-2014-1266 aka "goto fail" */
+int fn_6 (int a, int b, int c)
+{
+	int err;
+
+	/* ... */
+	if ((err = foo (a)) != 0)
+		goto fail;
+	if ((err = foo (b)) != 0) /* { dg-message "2: this 'if' clause does not guard..." } */
+		goto fail;
+		goto fail; /* { dg-message "3: ...this statement, but the latter is indented as if it does" } */
+	if ((err = foo (c)) != 0)
+		goto fail;
+	/* ... */
+
+/* { dg-begin-multiline-output "" }
+  if ((err = foo (b)) != 0)
+  ^~
+   { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+   goto fail;
+   ^~~~
+   { dg-end-multiline-output "" } */
+
+fail:
+	return err;
+}
+
+#define FOR_EACH(VAR, START, STOP) \
+  for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-warning "3: this 'for' clause does not guard..." } */
+
+void fn_14 (void)
+{
+  int i;
+  FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
+    foo (i);
+    bar (i, i); /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
+
+/* { dg-begin-multiline-output "" }
+   for ((VAR) = (START); (VAR) < (STOP); (VAR++))
+   ^
+   { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+   FOR_EACH (i, 0, 10)
+   ^~~~~~~~
+   { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+     bar (i, i);
+     ^~~
+   { dg-end-multiline-output "" } */
+}
+#undef FOR_EACH
diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
index 25db8fe..70e60b9 100644
--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
@@ -12,17 +12,17 @@ int
 fn_1 (int flag)
 {
   int x = 4, y = 5;
-  if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" } */
+  if (flag) /* { dg-warning "3: this 'if' clause does not guard..." } */
     x = 3;
-    y = 2; /* { dg-warning "statement is indented as if it were guarded by..." } */
+    y = 2; /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
   return x * y;
 }
 
 int
 fn_2 (int flag, int x, int y)
 {
-  if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" } */
-    x++; y++; /* { dg-warning "statement is indented as if it were guarded by..." } */
+  if (flag) /* { dg-warning "3: this 'if' clause does not guard..." } */
+    x++; y++; /* { dg-message "10: ...this statement, but the latter is indented as if it does" } */
 
   return x * y;
 }
@@ -33,9 +33,9 @@ fn_3 (int flag)
   int x = 4, y = 5;
   if (flag)
     x = 3;
-  else /* { dg-message "3: ...this 'else' clause, but it is not" } */
+  else /* { dg-warning "3: this 'else' clause does not guard..." } */
     x = 2;
-    y = 2; /* { dg-warning "statement is indented as if it were guarded by..." } */
+    y = 2; /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
   return x * y;
 }
 
@@ -43,18 +43,18 @@ void
 fn_4 (double *a, double *b, double *c)
 {
   int i = 0;
-  while (i < 10) /* { dg-message "3: ...this 'while' clause, but it is not" } */
+  while (i < 10) /* { dg-warning "3: this 'while' clause does not guard..." } */
     a[i] = b[i] * c[i];
-    i++; /* { dg-warning "statement is indented as if it were guarded by..." } */
+    i++; /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
 }
 
 void
 fn_5 (double *a, double *b, double *sum, double *prod)
 {
   int i = 0;
-  for (i = 0; i < 10; i++) /* { dg-output "3: ...this 'for' clause, but it is not" } */
+  for (i = 0; i < 10; i++) /* { dg-warning "3: this 'for' clause does not guard..." } */
     sum[i] = a[i] * b[i];
-    prod[i] = a[i] * b[i]; /* { dg-warning "statement is indented as if it were guarded by..." } */
+    prod[i] = a[i] * b[i]; /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
 }
 
 /* Based on CVE-2014-1266 aka "goto fail" */
@@ -65,9 +65,9 @@ int fn_6 (int a, int b, int c)
 	/* ... */
 	if ((err = foo (a)) != 0)
 		goto fail;
-	if ((err = foo (b)) != 0) /* { dg-message "2: ...this 'if' clause, but it is not" } */
+	if ((err = foo (b)) != 0) /* { dg-message "2: this 'if' clause does not guard..." } */
 		goto fail;
-		goto fail; /* { dg-warning "statement is indented as if it were guarded by..." } */
+		goto fail; /* { dg-message "3: ...this statement, but the latter is indented as if it does" } */
 	if ((err = foo (c)) != 0)
 		goto fail;
 	/* ... */
@@ -80,8 +80,8 @@ int fn_7 (int p, int q, int r, int s, int t)
 {
   if (bar (p, q))
     {
-      if (p) /* { dg-message "7: ...this 'if' clause, but it is not" } */
-        q++; r++; /* { dg-warning "statement is indented as if it were guarded by..." } */
+      if (p) /* { dg-message "7: this 'if' clause does not guard..." } */
+        q++; r++; /* { dg-message "14: ...this statement, but the latter is indented as if it does" } */
       t++;
     }
   return p + q + r + s + t;
@@ -95,20 +95,20 @@ int fn_8 (int a, int b, int c)
 
 void fn_9 (int flag)
 {
-  if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" } */
+  if (flag) /* { dg-warning "3: this 'if' clause does not guard..." } */
     foo (0);
-    foo (1); /* { dg-warning "statement is indented as if it were guarded by..." } */
+    foo (1); /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
 }
 
 void fn_10 (int flag)
 {
-  if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" } */
+  if (flag) /* { dg-warning "3: this 'if' clause does not guard..." } */
     if (flag / 2)
       {
         foo (0);
         foo (1);
       }
-    foo (2); /* { dg-warning "statement is indented as if it were guarded by..." } */
+    foo (2); /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
   foo (3);
 }
 
@@ -116,48 +116,48 @@ void fn_11 (void)
 {
   if (flagA)
     if (flagB)
-      if (flagC) /* { dg-message "7: ...this 'if' clause, but it is not" } */
+      if (flagC) /* { dg-message "7: this 'if' clause does not guard..." } */
         foo (0);
-        bar (1, 2); /* { dg-warning "statement is indented as if it were guarded by..." } */
+        bar (1, 2); /* { dg-message "9: ...this statement, but the latter is indented as if it does" } */
 }
 
 void fn_12 (void)
 {
   if (flagA)
-    if (flagB) /* { dg-message "5: ...this 'if' clause, but it is not" } */
+    if (flagB) /* { dg-message "5: this 'if' clause does not guard..." } */
       if (flagC)
         foo (0);
-      bar (1, 2); /* { dg-warning "statement is indented as if it were guarded by..." } */
+      bar (1, 2); /* { dg-message "7: ...this statement, but the latter is indented as if it does" } */
 }
 
 void fn_13 (void)
 {
-  if (flagA) /* { dg-message "3: ...this 'if' clause, but it is not" } */
+  if (flagA) /* { dg-warning "3: this 'if' clause does not guard..." } */
     if (flagB)
       if (flagC)
         foo (0);
-    bar (1, 2); /* { dg-warning "statement is indented as if it were guarded by..." } */
+    bar (1, 2); /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
 }
 
 #define FOR_EACH(VAR, START, STOP) \
-  for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-message "3: ...this 'for' clause, but it is not" } */
+  for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-warning "3: this 'for' clause does not guard..." } */
 
 void fn_14 (void)
 {
   int i;
-  FOR_EACH (i, 0, 10) /* { dg-message "3: in expansion of macro" } */
+  FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
     foo (i);
-    bar (i, i); /* { dg-warning "statement is indented as if it were guarded by..." } */
+    bar (i, i); /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
 }
 #undef FOR_EACH
 
-#define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-message "36: ...this 'for' clause, but it is not" } */
+#define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-message "36: this 'for' clause does not guard..." } */
 void fn_15 (void)
 {
   int i;
-  FOR_EACH (i, 0, 10) /* { dg-message "3: in expansion of macro" } */
+  FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
     foo (i);
-    bar (i, i); /* { dg-warning "statement is indented as if it were guarded by..." } */
+    bar (i, i); /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
 }
 #undef FOR_EACH
 
@@ -166,9 +166,9 @@ void fn_16_spaces (void)
   int i;
   for (i = 0; i < 10; i++)
     while (flagA)
-      if (flagB) /* { dg-message "7: ...this 'if' clause, but it is not" } */
+      if (flagB) /* { dg-message "7: this 'if' clause does not guard..." } */
         foo (0);
-        foo (1); /* { dg-warning "statement is indented as if it were guarded by..." } */
+        foo (1); /* { dg-message "9: ...this statement, but the latter is indented as if it does" } */
 }
 
 void fn_16_tabs (void)
@@ -176,49 +176,49 @@ void fn_16_tabs (void)
   int i;
   for (i = 0; i < 10; i++)
     while (flagA)
-      if (flagB) /* { dg-message "7: ...this 'if' clause, but it is not" } */
+      if (flagB) /* { dg-message "7: this 'if' clause does not guard..." } */
 	foo (0);
-	foo (1);/* { dg-warning "statement is indented as if it were guarded by..." } */
+	foo (1);/* { dg-message "2: ...this statement, but the latter is indented as if it does" } */
 }
 
 void fn_17_spaces (void)
 {
   int i;
-  for (i = 0; i < 10; i++) /* { dg-message "3: ...this 'for' clause, but it is not" } */
+  for (i = 0; i < 10; i++) /* { dg-warning "3: this 'for' clause does not guard..." } */
     while (flagA)
       if (flagB)
         foo (0);
-    foo (1);/* { dg-warning "statement is indented as if it were guarded by..." } */
+    foo (1);/* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
 }
 
 void fn_17_tabs (void)
 {
   int i;
-  for (i = 0; i < 10; i++) /* { dg-message "3: ...this 'for' clause, but it is not" } */
+  for (i = 0; i < 10; i++) /* { dg-warning "3: this 'for' clause does not guard..." } */
     while (flagA)
       if (flagB)
 	foo (0);
-    foo (1);/* { dg-warning "statement is indented as if it were guarded by..." } */
+    foo (1);/* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
 }
 
 void fn_18_spaces (void)
 {
   int i;
   for (i = 0; i < 10; i++)
-    while (flagA) /* { dg-message "5: ...this 'while' clause, but it is not" } */
+    while (flagA) /* { dg-message "5: this 'while' clause does not guard..." } */
       if (flagB)
         foo (0);
-      foo (1);/* { dg-warning "statement is indented as if it were guarded by..." } */
+      foo (1);/* { dg-message "7: ...this statement, but the latter is indented as if it does" } */
 }
 
 void fn_18_tabs (void)
 {
   int i;
   for (i = 0; i < 10; i++)
-    while (flagA) /* { dg-message "5: ...this 'while' clause, but it is not" } */
+    while (flagA) /* { dg-message "5: this 'while' clause does not guard..." } */
       if (flagB)
 	foo (0);
-      foo (1);/* { dg-warning "statement is indented as if it were guarded by..." } */
+      foo (1);/* { dg-message "7: ...this statement, but the latter is indented as if it does" } */
 }
 
 /* This shouldn't lead to a warning.  */
@@ -701,108 +701,108 @@ fn_37 (void)
   int i;
 
 #define EMPTY
-#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP; VAR++)
+#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP; VAR++) /* { dg-warning "this 'for' clause" } */
 
-  while (flagA); /* { dg-message "3: ...this 'while' clause" } */
-    foo (0); /* { dg-warning "statement is indented as if" } */
+  while (flagA); /* { dg-warning "3: this 'while' clause" } */
+    foo (0); /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
 
   if (flagA)
     ;
-  else if (flagB); /* { dg-message "8: ...this 'if' clause" } */
-    foo (0); /* { dg-warning "statement is indented as if" } */
-  while (flagA) /* { dg-message "3: ...this 'while' clause" } */
+  else if (flagB); /* { dg-warning "8: this 'if' clause" } */
+    foo (0); /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
+  while (flagA) /* { dg-warning "3: this 'while' clause" } */
     /* blah */;
-    foo (0); /* { dg-warning "statement is indented as if" } */
+    foo (0); /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
 
   if (flagA)
     ;
-  else if (flagB) /* { dg-message "8: ...this 'if' clause" } */
+  else if (flagB) /* { dg-warning "8: this 'if' clause" } */
     foo (1);
-    foo (2); /* { dg-warning "statement is indented as if" } */
+    foo (2); /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
 
   if (flagA)
     foo (1);
-  else if (flagB) /* { dg-message "8: ...this 'if' clause" } */
+  else if (flagB) /* { dg-warning "8: this 'if' clause" } */
     foo (2);
-    foo (3); /* { dg-warning "statement is indented as if" } */
+    foo (3); /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
 
-  if (flagB) /* { dg-message "3: ...this 'if' clause" } */
+  if (flagB) /* { dg-warning "3: this 'if' clause" } */
     /* blah */;
-    { /* { dg-warning "statement is indented as if" } */
+    { /* { dg-message "5: ...this statement, but the latter is indented as if it does" } */
       foo (0);
     }
 
-  if (flagB) /* { dg-message "3: ...this 'if' clause" } */
+  if (flagB) /* { dg-warning "3: this 'if' clause" } */
     /* blah */;
-   { /* { dg-warning "statement is indented as if" } */
+   { /* { dg-message "4: ...this statement, but the latter is indented as if it does" } */
      foo (0);
    }
 
 
   if (flagB)
     ;
-  else; foo (0); /* { dg-warning "statement is indented as if" } */
+  else; foo (0); /* { dg-warning "3: this 'else' clause" } */
 
-  if (flagC); foo (2); /* { dg-warning "statement is indented as if" } */
+  if (flagC); foo (2); /* { dg-warning "3: this 'if' clause" } */
 
-  if (flagA)
-    ; /* blah */ { /* { dg-warning "statement is indented as if" } */
+  if (flagA) /* { dg-warning "3: this 'if' clause" } */
+    ; /* blah */ { /* { dg-message "18: ...this statement" } */
       foo (1);
     }
 
-  if (flagB) ; /* { dg-message "3: ...this 'if' clause" } */
-    return; /* { dg-warning "statement is indented as if" } */
+  if (flagB) ; /* { dg-warning "3: this 'if' clause" } */
+    return; /* { dg-message "5: ...this statement" } */
 
-  if (flagB) EMPTY; /* { dg-message "3: ...this 'if' clause" } */
-    foo (1); /* { dg-warning "statement is indented as if" } */
+  if (flagB) EMPTY; /* { dg-warning "3: this 'if' clause" } */
+    foo (1); /* { dg-message "5: ...this statement" } */
 
-  for (i = 0; i < 10; i++); /* { dg-message "3: ...this 'for' clause" } */
-    foo (2); /* { dg-warning "statement is indented as if" } */
+  for (i = 0; i < 10; i++); /* { dg-warning "3: this 'for' clause" } */
+    foo (2); /* { dg-message "5: ...this statement" } */
 
-  FOR_EACH (i, 0, 10);
-    foo (2); /* { dg-warning "statement is indented as if" } */
+  FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
+    foo (2); /* { dg-message "5: ...this statement" } */
 
-  FOR_EACH (i, 0, 10);
-    { /* { dg-warning "statement is indented as if" } */
+  FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
+    { /* { dg-message "5: ...this statement" } */
       foo (3);
     }
 
-  FOR_EACH (i, 0, 10);
-  { /* { dg-warning "statement is indented as if" } */
+  FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
+  { /* { dg-message "3: ...this statement" } */
     foo (3);
   }
 
-  while (i++); { /* { dg-warning "statement is indented as if" } */
+  while (i++); { /* { dg-warning "3: this 'while' clause" } */
     foo (3);
   }
 
-  if (i++); { /* { dg-warning "statement is indented as if" } */
+  if (i++); { /* { dg-warning "3: this 'if' clause" } */
     foo (3);
   }
 
   if (flagA) {
     foo (1);
-  } else /* { dg-message "5: ...this 'else' clause" } */
+  } else /* { dg-warning "5: this 'else' clause" } */
     if (flagB)
        foo (2);
-    foo (3); /* { dg-warning "statement is indented as if" } */
+    foo (3); /* { dg-message "5: ...this statement" } */
 
   if (flagA)
     foo (1);
-  else if (flagB); /* { dg-message "8: ...this 'if' clause" } */
-    foo (2); /* { dg-warning "statement is indented as if" } */
+  else if (flagB); /* { dg-warning "8: this 'if' clause" } */
+    foo (2); /* { dg-message "5: ...this statement" } */
 
-  for (i = 0; /* { dg-message "3: ...this 'for' clause" } */
+  for (i = 0; /* { dg-warning "3: this 'for' clause" } */
        i < 10;
        i++);
-    foo (i); /* { dg-warning "statement is indented as if" } */
+    foo (i); /* { dg-message "5: ...this statement" } */
 
   if (flagA)
   {
     foo (1);
   }
-  else if (flagB); /* { dg-message "8: ...this 'if' clause" } */
-  { /* { dg-warning "statement is indented as if" } */
+  else if (flagB); /* { dg-warning "8: this 'if' clause" } */
+  { /* { dg-message "3: ...this statement" } */
     foo (2);
   }
 
diff --git a/gcc/testsuite/gcc.dg/plugin/location-overflow-test-2.c b/gcc/testsuite/gcc.dg/plugin/location-overflow-test-2.c
index c8b45b6..eb37519 100644
--- a/gcc/testsuite/gcc.dg/plugin/location-overflow-test-2.c
+++ b/gcc/testsuite/gcc.dg/plugin/location-overflow-test-2.c
@@ -20,7 +20,7 @@ int
 fn_1 (int flag)
 {
   int foo = 4, bar = 5;
-  if (flag) foo = 3; bar = 2; /* { dg-warning "indented" } */
+  if (flag) foo = 3; bar = 2; /* { dg-warning "this .if." } */
   return foo * bar;
 }
 
-- 
1.8.5.3

Re: [PATCH] PR c/69993: improvements to wording of -Wmisleading-indentation

[Richard Biener]

On March 1, 2016 7:51:01 PM GMT+01:00, David Malcolm <dmalcolm@redhat.com> wrote:
>The wording of our output from -Wmisleading-indentation is rather
>confusing, as noted by Reddit user "sysop073" here:
>https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmisleadingindentation_vs_goto_fail/d0eonwd
>
>> The way they split up the warning looks designed to trick you.
>> sslKeyExchange.c:631:8: warning: statement is indented as if it were
>guarded by... [-Wmisleading-indentation]
>>         goto fail;
>>         ^~~~
>> sslKeyExchange.c:629:4: note: ...this 'if' clause, but it is not
>>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>>     ^~
>> You read the first half and it sounds like goto fail; is guarding
>something. Why would it not be:
>> sslKeyExchange.c:631:8: warning: statement is wrongly indented...
>[-Wmisleading-indentation]
>>         goto fail;
>>         ^~~~
>> sslKeyExchange.c:629:4: note: ...as if it were guarded by this 'if'
>clause
>>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>>     ^~
>
>I agree that the current wording is suboptimal; certainly the wording
>would be much clearer if the wording of the "warning" only spoke about
>the
>statement in question, and the "note"/inform should then talk about the
>not-really-guarding guard.
>
>One rewording could be:
>
>sslKeyExchange.c:631:8: warning: statement is misleadingly indented...
>[-Wmisleading-indentation]
>        goto fail;
>        ^~~~
>sslKeyExchange.c:629:4: note: ...as if it were guarded by this 'if'
>clause, but it is not
>    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>    ^~
>
>However another Reddit user ("ksion") noted here:
>https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmisleadingindentation_vs_goto_fail/d0eqyih
>that:
>> This is just passive voice, there is nothing tricky about it.
>> What I find more confusing -- and what your fix preserves -- is the
>> reversed order of offending lines of code in the source file and the
>message.
>>
>> I'd rather go with something like this:
>> sslKeyExchange.c:629:4: warning: indentation of a statement below
>this 'if' clause... [-Wmisleading-indentation]
>>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>>     ^~
>> sslKeyExchange.c:631:8: note: ...suggests it is guarded by the 'if'
>clause, but it's not
>>         goto fail;
>>         ^~~~
>> You can even see how the indentation is wrong in the very error
>message.
>
>which suggests reversing the order of the messages, so that they appear
>in "source" order.
>
>I think this is a big improvement in the readability of the warning.
>
>The attached patch implements such a change, so that the warning is
>issued on the supposed guard clause, followed by the note on the
>statement that isn't really guarded.
>
>Some examples:
>
>Wmisleading-indentation-3.c:18:3: warning: this 'for' clause does not
>guard... [-Wmisleading-indentation]
>   for (i = 0; i < 10; i++)
>   ^~~
>Wmisleading-indentation-3.c:20:5: note: ...this statement, but the
>latter is indented as if it does
>     prod[i] = a[i] * b[i];
>     ^~~~
>Wmisleading-indentation-3.c: In function 'fn_6':
>Wmisleading-indentation-3.c:39:2: warning: this 'if' clause does not
>guard... [-Wmisleading-indentation]
>  if ((err = foo (b)) != 0)
>  ^~
>Wmisleading-indentation-3.c:41:3: note: ...this statement, but the
>latter is indented as if it does
>   goto fail;
>   ^~~~
>
>I'm not totally convinced by my new wording; maybe the note could
>also mention the kind of clause ('if'/'while'/'else'/'for') for
>clarity, maybe something like:
>
>Wmisleading-indentation-3.c: In function 'fn_6':
>Wmisleading-indentation-3.c:39:2: warning: this 'if' clause does not
>guard... [-Wmisleading-indentation]
>  if ((err = foo (b)) != 0)
>  ^~
>Wmisleading-indentation-3.c:41:3: note: ...this statement, but the
>latter is misleadingly indented
>as if it is guarded by the 'if'
>   goto fail;
>   ^~~~
>
>Also, it's slightly clunkier when it comes to macros, e.g.:
>
>Wmisleading-indentation-3.c: In function 'fn_14':
>Wmisleading-indentation-3.c:60:3: warning: this 'for' clause does not
>guard... [-Wmisleading-indentation]
>   for ((VAR) = (START); (VAR) < (STOP); (VAR++))
>   ^
>Wmisleading-indentation-3.c:65:3: note: in expansion of macro
>'FOR_EACH'
>   FOR_EACH (i, 0, 10)
>   ^~~~~~~~
>Wmisleading-indentation-3.c:67:5: note: ...this statement, but the
>latter is indented as if it does
>     bar (i, i);
>     ^~~
>
>That said, the reordering idea is something I'd like to do for GCC 6.
>Failing that, there's the tweak to the wording suggested at the top.
>
>OK for trunk? (assuming we can agree on the wording, and that the
>latest
>version passes testing)

OK if others don't disagree.

Richard.

>gcc/c-family/ChangeLog:
>	PR c/69993
>	* c-indentation.c (warn_for_misleading_indentation): Rewrite the
>	diagnostic text, reversing the order of the warning and note so
>	that they appear in source order.
>
>gcc/testsuite/ChangeLog:
>	PR c/69993
>	* c-c++-common/Wmisleading-indentation-3.c: New test, based on
>	Wmisleading-indentation.c.
>	* c-c++-common/Wmisleading-indentation.c: Update thoughout to
>	reflect change to diagnostic text and order of messages.
>	* gcc.dg/plugin/location-overflow-test-2.c: Likewise.
>---
> gcc/c-family/c-indentation.c                       |  10 +-
> .../c-c++-common/Wmisleading-indentation-3.c       |  82 ++++++++++
>.../c-c++-common/Wmisleading-indentation.c         | 166
>++++++++++-----------
> .../gcc.dg/plugin/location-overflow-test-2.c       |   2 +-
> 4 files changed, 171 insertions(+), 89 deletions(-)
>create mode 100644
>gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
>
>diff --git a/gcc/c-family/c-indentation.c
>b/gcc/c-family/c-indentation.c
>index 521f992..1325567 100644
>--- a/gcc/c-family/c-indentation.c
>+++ b/gcc/c-family/c-indentation.c
>@@ -579,10 +579,10 @@ warn_for_misleading_indentation (const
>token_indent_info &guard_tinfo,
> 					      body_tinfo,
> 					      next_tinfo))
>     {
>-      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",
>-		guard_tinfo_to_string (guard_tinfo));
>+      if (warning_at (guard_tinfo.location,
>OPT_Wmisleading_indentation,
>+		      "this %qs clause does not guard...",
>+		      guard_tinfo_to_string (guard_tinfo)))
>+        inform (next_tinfo.location,
>+		"...this statement, but the latter is indented as if it does");
>     }
> }
>diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
>b/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
>new file mode 100644
>index 0000000..7c14658
>--- /dev/null
>+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
>@@ -0,0 +1,82 @@
>+/* Verify -Wmisleading-indentation with source-printing.
>+   This is a subset of Wmisleading-indentation.c.  */
>+
>+/* { dg-options "-Wmisleading-indentation -fdiagnostics-show-caret" }
>*/
>+/* { dg-do compile } */
>+
>+extern int foo (int);
>+extern int bar (int, int);
>+extern int flagA;
>+extern int flagB;
>+extern int flagC;
>+extern int flagD;
>+
>+void
>+fn_5 (double *a, double *b, double *sum, double *prod)
>+{
>+  int i = 0;
>+  for (i = 0; i < 10; i++) /* { dg-warning "3: this 'for' clause does
>not guard..." } */
>+    sum[i] = a[i] * b[i];
>+    prod[i] = a[i] * b[i]; /* { dg-message "5: ...this statement, but
>the latter is indented as if it does" } */
>+/* { dg-begin-multiline-output "" }
>+   for (i = 0; i < 10; i++)
>+   ^~~
>+   { dg-end-multiline-output "" } */
>+/* { dg-begin-multiline-output "" }
>+     prod[i] = a[i] * b[i];
>+     ^~~~
>+   { dg-end-multiline-output "" } */
>+}
>+
>+/* Based on CVE-2014-1266 aka "goto fail" */
>+int fn_6 (int a, int b, int c)
>+{
>+	int err;
>+
>+	/* ... */
>+	if ((err = foo (a)) != 0)
>+		goto fail;
>+	if ((err = foo (b)) != 0) /* { dg-message "2: this 'if' clause does
>not guard..." } */
>+		goto fail;
>+		goto fail; /* { dg-message "3: ...this statement, but the latter is
>indented as if it does" } */
>+	if ((err = foo (c)) != 0)
>+		goto fail;
>+	/* ... */
>+
>+/* { dg-begin-multiline-output "" }
>+  if ((err = foo (b)) != 0)
>+  ^~
>+   { dg-end-multiline-output "" } */
>+/* { dg-begin-multiline-output "" }
>+   goto fail;
>+   ^~~~
>+   { dg-end-multiline-output "" } */
>+
>+fail:
>+	return err;
>+}
>+
>+#define FOR_EACH(VAR, START, STOP) \
>+  for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-warning "3:
>this 'for' clause does not guard..." } */
>+
>+void fn_14 (void)
>+{
>+  int i;
>+  FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro
>.FOR_EACH." } */
>+    foo (i);
>+    bar (i, i); /* { dg-message "5: ...this statement, but the latter
>is indented as if it does" } */
>+
>+/* { dg-begin-multiline-output "" }
>+   for ((VAR) = (START); (VAR) < (STOP); (VAR++))
>+   ^
>+   { dg-end-multiline-output "" } */
>+/* { dg-begin-multiline-output "" }
>+   FOR_EACH (i, 0, 10)
>+   ^~~~~~~~
>+   { dg-end-multiline-output "" } */
>+/* { dg-begin-multiline-output "" }
>+     bar (i, i);
>+     ^~~
>+   { dg-end-multiline-output "" } */
>+}
>+#undef FOR_EACH
>diff --git a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
>b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
>index 25db8fe..70e60b9 100644
>--- a/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
>+++ b/gcc/testsuite/c-c++-common/Wmisleading-indentation.c
>@@ -12,17 +12,17 @@ int
> fn_1 (int flag)
> {
>   int x = 4, y = 5;
>-  if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" }
>*/
>+  if (flag) /* { dg-warning "3: this 'if' clause does not guard..." }
>*/
>     x = 3;
>-    y = 2; /* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+    y = 2; /* { dg-message "5: ...this statement, but the latter is
>indented as if it does" } */
>   return x * y;
> }
> 
> int
> fn_2 (int flag, int x, int y)
> {
>-  if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" }
>*/
>-    x++; y++; /* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+  if (flag) /* { dg-warning "3: this 'if' clause does not guard..." }
>*/
>+    x++; y++; /* { dg-message "10: ...this statement, but the latter
>is indented as if it does" } */
> 
>   return x * y;
> }
>@@ -33,9 +33,9 @@ fn_3 (int flag)
>   int x = 4, y = 5;
>   if (flag)
>     x = 3;
>-  else /* { dg-message "3: ...this 'else' clause, but it is not" } */
>+  else /* { dg-warning "3: this 'else' clause does not guard..." } */
>     x = 2;
>-    y = 2; /* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+    y = 2; /* { dg-message "5: ...this statement, but the latter is
>indented as if it does" } */
>   return x * y;
> }
> 
>@@ -43,18 +43,18 @@ void
> fn_4 (double *a, double *b, double *c)
> {
>   int i = 0;
>-  while (i < 10) /* { dg-message "3: ...this 'while' clause, but it is
>not" } */
>+  while (i < 10) /* { dg-warning "3: this 'while' clause does not
>guard..." } */
>     a[i] = b[i] * c[i];
>-    i++; /* { dg-warning "statement is indented as if it were guarded
>by..." } */
>+    i++; /* { dg-message "5: ...this statement, but the latter is
>indented as if it does" } */
> }
> 
> void
> fn_5 (double *a, double *b, double *sum, double *prod)
> {
>   int i = 0;
>-  for (i = 0; i < 10; i++) /* { dg-output "3: ...this 'for' clause,
>but it is not" } */
>+  for (i = 0; i < 10; i++) /* { dg-warning "3: this 'for' clause does
>not guard..." } */
>     sum[i] = a[i] * b[i];
>-    prod[i] = a[i] * b[i]; /* { dg-warning "statement is indented as
>if it were guarded by..." } */
>+    prod[i] = a[i] * b[i]; /* { dg-message "5: ...this statement, but
>the latter is indented as if it does" } */
> }
> 
> /* Based on CVE-2014-1266 aka "goto fail" */
>@@ -65,9 +65,9 @@ int fn_6 (int a, int b, int c)
> 	/* ... */
> 	if ((err = foo (a)) != 0)
> 		goto fail;
>-	if ((err = foo (b)) != 0) /* { dg-message "2: ...this 'if' clause,
>but it is not" } */
>+	if ((err = foo (b)) != 0) /* { dg-message "2: this 'if' clause does
>not guard..." } */
> 		goto fail;
>-		goto fail; /* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+		goto fail; /* { dg-message "3: ...this statement, but the latter is
>indented as if it does" } */
> 	if ((err = foo (c)) != 0)
> 		goto fail;
> 	/* ... */
>@@ -80,8 +80,8 @@ int fn_7 (int p, int q, int r, int s, int t)
> {
>   if (bar (p, q))
>     {
>-      if (p) /* { dg-message "7: ...this 'if' clause, but it is not" }
>*/
>-        q++; r++; /* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+      if (p) /* { dg-message "7: this 'if' clause does not guard..." }
>*/
>+        q++; r++; /* { dg-message "14: ...this statement, but the
>latter is indented as if it does" } */
>       t++;
>     }
>   return p + q + r + s + t;
>@@ -95,20 +95,20 @@ int fn_8 (int a, int b, int c)
> 
> void fn_9 (int flag)
> {
>-  if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" }
>*/
>+  if (flag) /* { dg-warning "3: this 'if' clause does not guard..." }
>*/
>     foo (0);
>-    foo (1); /* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+    foo (1); /* { dg-message "5: ...this statement, but the latter is
>indented as if it does" } */
> }
> 
> void fn_10 (int flag)
> {
>-  if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" }
>*/
>+  if (flag) /* { dg-warning "3: this 'if' clause does not guard..." }
>*/
>     if (flag / 2)
>       {
>         foo (0);
>         foo (1);
>       }
>-    foo (2); /* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+    foo (2); /* { dg-message "5: ...this statement, but the latter is
>indented as if it does" } */
>   foo (3);
> }
> 
>@@ -116,48 +116,48 @@ void fn_11 (void)
> {
>   if (flagA)
>     if (flagB)
>-      if (flagC) /* { dg-message "7: ...this 'if' clause, but it is
>not" } */
>+      if (flagC) /* { dg-message "7: this 'if' clause does not
>guard..." } */
>         foo (0);
>-        bar (1, 2); /* { dg-warning "statement is indented as if it
>were guarded by..." } */
>+        bar (1, 2); /* { dg-message "9: ...this statement, but the
>latter is indented as if it does" } */
> }
> 
> void fn_12 (void)
> {
>   if (flagA)
>-    if (flagB) /* { dg-message "5: ...this 'if' clause, but it is not"
>} */
>+    if (flagB) /* { dg-message "5: this 'if' clause does not guard..."
>} */
>       if (flagC)
>         foo (0);
>-      bar (1, 2); /* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+      bar (1, 2); /* { dg-message "7: ...this statement, but the
>latter is indented as if it does" } */
> }
> 
> void fn_13 (void)
> {
>-  if (flagA) /* { dg-message "3: ...this 'if' clause, but it is not" }
>*/
>+  if (flagA) /* { dg-warning "3: this 'if' clause does not guard..." }
>*/
>     if (flagB)
>       if (flagC)
>         foo (0);
>-    bar (1, 2); /* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+    bar (1, 2); /* { dg-message "5: ...this statement, but the latter
>is indented as if it does" } */
> }
> 
> #define FOR_EACH(VAR, START, STOP) \
>-  for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-message "3:
>...this 'for' clause, but it is not" } */
>+  for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-warning "3:
>this 'for' clause does not guard..." } */
> 
> void fn_14 (void)
> {
>   int i;
>-  FOR_EACH (i, 0, 10) /* { dg-message "3: in expansion of macro" } */
>+  FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro
>.FOR_EACH." } */
>     foo (i);
>-    bar (i, i); /* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+    bar (i, i); /* { dg-message "5: ...this statement, but the latter
>is indented as if it does" } */
> }
> #undef FOR_EACH
> 
>-#define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) <
>(STOP); (VAR++)) /* { dg-message "36: ...this 'for' clause, but it is
>not" } */
>+#define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) <
>(STOP); (VAR++)) /* { dg-message "36: this 'for' clause does not
>guard..." } */
> void fn_15 (void)
> {
>   int i;
>-  FOR_EACH (i, 0, 10) /* { dg-message "3: in expansion of macro" } */
>+  FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro
>.FOR_EACH." } */
>     foo (i);
>-    bar (i, i); /* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+    bar (i, i); /* { dg-message "5: ...this statement, but the latter
>is indented as if it does" } */
> }
> #undef FOR_EACH
> 
>@@ -166,9 +166,9 @@ void fn_16_spaces (void)
>   int i;
>   for (i = 0; i < 10; i++)
>     while (flagA)
>-      if (flagB) /* { dg-message "7: ...this 'if' clause, but it is
>not" } */
>+      if (flagB) /* { dg-message "7: this 'if' clause does not
>guard..." } */
>         foo (0);
>-        foo (1); /* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+        foo (1); /* { dg-message "9: ...this statement, but the latter
>is indented as if it does" } */
> }
> 
> void fn_16_tabs (void)
>@@ -176,49 +176,49 @@ void fn_16_tabs (void)
>   int i;
>   for (i = 0; i < 10; i++)
>     while (flagA)
>-      if (flagB) /* { dg-message "7: ...this 'if' clause, but it is
>not" } */
>+      if (flagB) /* { dg-message "7: this 'if' clause does not
>guard..." } */
> 	foo (0);
>-	foo (1);/* { dg-warning "statement is indented as if it were guarded
>by..." } */
>+	foo (1);/* { dg-message "2: ...this statement, but the latter is
>indented as if it does" } */
> }
> 
> void fn_17_spaces (void)
> {
>   int i;
>-  for (i = 0; i < 10; i++) /* { dg-message "3: ...this 'for' clause,
>but it is not" } */
>+  for (i = 0; i < 10; i++) /* { dg-warning "3: this 'for' clause does
>not guard..." } */
>     while (flagA)
>       if (flagB)
>         foo (0);
>-    foo (1);/* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+    foo (1);/* { dg-message "5: ...this statement, but the latter is
>indented as if it does" } */
> }
> 
> void fn_17_tabs (void)
> {
>   int i;
>-  for (i = 0; i < 10; i++) /* { dg-message "3: ...this 'for' clause,
>but it is not" } */
>+  for (i = 0; i < 10; i++) /* { dg-warning "3: this 'for' clause does
>not guard..." } */
>     while (flagA)
>       if (flagB)
> 	foo (0);
>-    foo (1);/* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+    foo (1);/* { dg-message "5: ...this statement, but the latter is
>indented as if it does" } */
> }
> 
> void fn_18_spaces (void)
> {
>   int i;
>   for (i = 0; i < 10; i++)
>-    while (flagA) /* { dg-message "5: ...this 'while' clause, but it
>is not" } */
>+    while (flagA) /* { dg-message "5: this 'while' clause does not
>guard..." } */
>       if (flagB)
>         foo (0);
>-      foo (1);/* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+      foo (1);/* { dg-message "7: ...this statement, but the latter is
>indented as if it does" } */
> }
> 
> void fn_18_tabs (void)
> {
>   int i;
>   for (i = 0; i < 10; i++)
>-    while (flagA) /* { dg-message "5: ...this 'while' clause, but it
>is not" } */
>+    while (flagA) /* { dg-message "5: this 'while' clause does not
>guard..." } */
>       if (flagB)
> 	foo (0);
>-      foo (1);/* { dg-warning "statement is indented as if it were
>guarded by..." } */
>+      foo (1);/* { dg-message "7: ...this statement, but the latter is
>indented as if it does" } */
> }
> 
> /* This shouldn't lead to a warning.  */
>@@ -701,108 +701,108 @@ fn_37 (void)
>   int i;
> 
> #define EMPTY
>-#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP;
>VAR++)
>+#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP;
>VAR++) /* { dg-warning "this 'for' clause" } */
> 
>-  while (flagA); /* { dg-message "3: ...this 'while' clause" } */
>-    foo (0); /* { dg-warning "statement is indented as if" } */
>+  while (flagA); /* { dg-warning "3: this 'while' clause" } */
>+    foo (0); /* { dg-message "5: ...this statement, but the latter is
>indented as if it does" } */
> 
>   if (flagA)
>     ;
>-  else if (flagB); /* { dg-message "8: ...this 'if' clause" } */
>-    foo (0); /* { dg-warning "statement is indented as if" } */
>-  while (flagA) /* { dg-message "3: ...this 'while' clause" } */
>+  else if (flagB); /* { dg-warning "8: this 'if' clause" } */
>+    foo (0); /* { dg-message "5: ...this statement, but the latter is
>indented as if it does" } */
>+  while (flagA) /* { dg-warning "3: this 'while' clause" } */
>     /* blah */;
>-    foo (0); /* { dg-warning "statement is indented as if" } */
>+    foo (0); /* { dg-message "5: ...this statement, but the latter is
>indented as if it does" } */
> 
>   if (flagA)
>     ;
>-  else if (flagB) /* { dg-message "8: ...this 'if' clause" } */
>+  else if (flagB) /* { dg-warning "8: this 'if' clause" } */
>     foo (1);
>-    foo (2); /* { dg-warning "statement is indented as if" } */
>+    foo (2); /* { dg-message "5: ...this statement, but the latter is
>indented as if it does" } */
> 
>   if (flagA)
>     foo (1);
>-  else if (flagB) /* { dg-message "8: ...this 'if' clause" } */
>+  else if (flagB) /* { dg-warning "8: this 'if' clause" } */
>     foo (2);
>-    foo (3); /* { dg-warning "statement is indented as if" } */
>+    foo (3); /* { dg-message "5: ...this statement, but the latter is
>indented as if it does" } */
> 
>-  if (flagB) /* { dg-message "3: ...this 'if' clause" } */
>+  if (flagB) /* { dg-warning "3: this 'if' clause" } */
>     /* blah */;
>-    { /* { dg-warning "statement is indented as if" } */
>+    { /* { dg-message "5: ...this statement, but the latter is
>indented as if it does" } */
>       foo (0);
>     }
> 
>-  if (flagB) /* { dg-message "3: ...this 'if' clause" } */
>+  if (flagB) /* { dg-warning "3: this 'if' clause" } */
>     /* blah */;
>-   { /* { dg-warning "statement is indented as if" } */
>+   { /* { dg-message "4: ...this statement, but the latter is indented
>as if it does" } */
>      foo (0);
>    }
> 
> 
>   if (flagB)
>     ;
>-  else; foo (0); /* { dg-warning "statement is indented as if" } */
>+  else; foo (0); /* { dg-warning "3: this 'else' clause" } */
> 
>-  if (flagC); foo (2); /* { dg-warning "statement is indented as if" }
>*/
>+  if (flagC); foo (2); /* { dg-warning "3: this 'if' clause" } */
> 
>-  if (flagA)
>-    ; /* blah */ { /* { dg-warning "statement is indented as if" } */
>+  if (flagA) /* { dg-warning "3: this 'if' clause" } */
>+    ; /* blah */ { /* { dg-message "18: ...this statement" } */
>       foo (1);
>     }
> 
>-  if (flagB) ; /* { dg-message "3: ...this 'if' clause" } */
>-    return; /* { dg-warning "statement is indented as if" } */
>+  if (flagB) ; /* { dg-warning "3: this 'if' clause" } */
>+    return; /* { dg-message "5: ...this statement" } */
> 
>-  if (flagB) EMPTY; /* { dg-message "3: ...this 'if' clause" } */
>-    foo (1); /* { dg-warning "statement is indented as if" } */
>+  if (flagB) EMPTY; /* { dg-warning "3: this 'if' clause" } */
>+    foo (1); /* { dg-message "5: ...this statement" } */
> 
>-  for (i = 0; i < 10; i++); /* { dg-message "3: ...this 'for' clause"
>} */
>-    foo (2); /* { dg-warning "statement is indented as if" } */
>+  for (i = 0; i < 10; i++); /* { dg-warning "3: this 'for' clause" }
>*/
>+    foo (2); /* { dg-message "5: ...this statement" } */
> 
>-  FOR_EACH (i, 0, 10);
>-    foo (2); /* { dg-warning "statement is indented as if" } */
>+  FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro
>.FOR_EACH." } */
>+    foo (2); /* { dg-message "5: ...this statement" } */
> 
>-  FOR_EACH (i, 0, 10);
>-    { /* { dg-warning "statement is indented as if" } */
>+  FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro
>.FOR_EACH." } */
>+    { /* { dg-message "5: ...this statement" } */
>       foo (3);
>     }
> 
>-  FOR_EACH (i, 0, 10);
>-  { /* { dg-warning "statement is indented as if" } */
>+  FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro
>.FOR_EACH." } */
>+  { /* { dg-message "3: ...this statement" } */
>     foo (3);
>   }
> 
>-  while (i++); { /* { dg-warning "statement is indented as if" } */
>+  while (i++); { /* { dg-warning "3: this 'while' clause" } */
>     foo (3);
>   }
> 
>-  if (i++); { /* { dg-warning "statement is indented as if" } */
>+  if (i++); { /* { dg-warning "3: this 'if' clause" } */
>     foo (3);
>   }
> 
>   if (flagA) {
>     foo (1);
>-  } else /* { dg-message "5: ...this 'else' clause" } */
>+  } else /* { dg-warning "5: this 'else' clause" } */
>     if (flagB)
>        foo (2);
>-    foo (3); /* { dg-warning "statement is indented as if" } */
>+    foo (3); /* { dg-message "5: ...this statement" } */
> 
>   if (flagA)
>     foo (1);
>-  else if (flagB); /* { dg-message "8: ...this 'if' clause" } */
>-    foo (2); /* { dg-warning "statement is indented as if" } */
>+  else if (flagB); /* { dg-warning "8: this 'if' clause" } */
>+    foo (2); /* { dg-message "5: ...this statement" } */
> 
>-  for (i = 0; /* { dg-message "3: ...this 'for' clause" } */
>+  for (i = 0; /* { dg-warning "3: this 'for' clause" } */
>        i < 10;
>        i++);
>-    foo (i); /* { dg-warning "statement is indented as if" } */
>+    foo (i); /* { dg-message "5: ...this statement" } */
> 
>   if (flagA)
>   {
>     foo (1);
>   }
>-  else if (flagB); /* { dg-message "8: ...this 'if' clause" } */
>-  { /* { dg-warning "statement is indented as if" } */
>+  else if (flagB); /* { dg-warning "8: this 'if' clause" } */
>+  { /* { dg-message "3: ...this statement" } */
>     foo (2);
>   }
> 
>diff --git a/gcc/testsuite/gcc.dg/plugin/location-overflow-test-2.c
>b/gcc/testsuite/gcc.dg/plugin/location-overflow-test-2.c
>index c8b45b6..eb37519 100644
>--- a/gcc/testsuite/gcc.dg/plugin/location-overflow-test-2.c
>+++ b/gcc/testsuite/gcc.dg/plugin/location-overflow-test-2.c
>@@ -20,7 +20,7 @@ int
> fn_1 (int flag)
> {
>   int foo = 4, bar = 5;
>-  if (flag) foo = 3; bar = 2; /* { dg-warning "indented" } */
>+  if (flag) foo = 3; bar = 2; /* { dg-warning "this .if." } */
>   return foo * bar;
> }
> 

Re: [PATCH] PR c/69993: improvements to wording of -Wmisleading-indentation

[Patrick Palka]

On Tue, Mar 1, 2016 at 1:51 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> The wording of our output from -Wmisleading-indentation is rather
> confusing, as noted by Reddit user "sysop073" here:
>  https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmisleadingindentation_vs_goto_fail/d0eonwd
>
>> The way they split up the warning looks designed to trick you.
>> sslKeyExchange.c:631:8: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
>>         goto fail;
>>         ^~~~
>> sslKeyExchange.c:629:4: note: ...this 'if' clause, but it is not
>>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>>     ^~
>> You read the first half and it sounds like goto fail; is guarding something. Why would it not be:
>> sslKeyExchange.c:631:8: warning: statement is wrongly indented... [-Wmisleading-indentation]
>>         goto fail;
>>         ^~~~
>> sslKeyExchange.c:629:4: note: ...as if it were guarded by this 'if' clause
>>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>>     ^~
>
> I agree that the current wording is suboptimal; certainly the wording
> would be much clearer if the wording of the "warning" only spoke about the
> statement in question, and the "note"/inform should then talk about the
> not-really-guarding guard.
>
> One rewording could be:
>
> sslKeyExchange.c:631:8: warning: statement is misleadingly indented... [-Wmisleading-indentation]
>         goto fail;
>         ^~~~
> sslKeyExchange.c:629:4: note: ...as if it were guarded by this 'if' clause, but it is not
>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>     ^~

I prefer this one because it makes it clear that the problem is with
the misleadingly-indented goto and not with the if.

>
> However another Reddit user ("ksion") noted here:
>   https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmisleadingindentation_vs_goto_fail/d0eqyih
> that:
>> This is just passive voice, there is nothing tricky about it.
>> What I find more confusing -- and what your fix preserves -- is the
>> reversed order of offending lines of code in the source file and the message.
>>
>> I'd rather go with something like this:
>> sslKeyExchange.c:629:4: warning: indentation of a statement below this 'if' clause... [-Wmisleading-indentation]
>>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>>     ^~
>> sslKeyExchange.c:631:8: note: ...suggests it is guarded by the 'if' clause, but it's not
>>         goto fail;
>>         ^~~~
>> You can even see how the indentation is wrong in the very error message.
>
> which suggests reversing the order of the messages, so that they appear
> in "source" order.
>
> I think this is a big improvement in the readability of the warning.

Using this wording order makes it seem that the problem is with the if
statement, because we emit a warning about it and then emit "only" a
note for the misleadingly-indented goto statement.

Re: [PATCH] PR c/69993: improvements to wording of -Wmisleading-indentation

[Patrick Palka]

On Tue, Mar 1, 2016 at 3:29 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Tue, Mar 1, 2016 at 1:51 PM, David Malcolm <dmalcolm@redhat.com> wrote:
>> The wording of our output from -Wmisleading-indentation is rather
>> confusing, as noted by Reddit user "sysop073" here:
>>  https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmisleadingindentation_vs_goto_fail/d0eonwd
>>
>>> The way they split up the warning looks designed to trick you.
>>> sslKeyExchange.c:631:8: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
>>>         goto fail;
>>>         ^~~~
>>> sslKeyExchange.c:629:4: note: ...this 'if' clause, but it is not
>>>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>>>     ^~
>>> You read the first half and it sounds like goto fail; is guarding something. Why would it not be:
>>> sslKeyExchange.c:631:8: warning: statement is wrongly indented... [-Wmisleading-indentation]
>>>         goto fail;
>>>         ^~~~
>>> sslKeyExchange.c:629:4: note: ...as if it were guarded by this 'if' clause
>>>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>>>     ^~
>>
>> I agree that the current wording is suboptimal; certainly the wording
>> would be much clearer if the wording of the "warning" only spoke about the
>> statement in question, and the "note"/inform should then talk about the
>> not-really-guarding guard.
>>
>> One rewording could be:
>>
>> sslKeyExchange.c:631:8: warning: statement is misleadingly indented... [-Wmisleading-indentation]
>>         goto fail;
>>         ^~~~
>> sslKeyExchange.c:629:4: note: ...as if it were guarded by this 'if' clause, but it is not
>>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>>     ^~
>
> I prefer this one because it makes it clear that the problem is with
> the misleadingly-indented goto and not with the if.
>
>>
>> However another Reddit user ("ksion") noted here:
>>   https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmisleadingindentation_vs_goto_fail/d0eqyih
>> that:
>>> This is just passive voice, there is nothing tricky about it.
>>> What I find more confusing -- and what your fix preserves -- is the
>>> reversed order of offending lines of code in the source file and the message.
>>>
>>> I'd rather go with something like this:
>>> sslKeyExchange.c:629:4: warning: indentation of a statement below this 'if' clause... [-Wmisleading-indentation]
>>>     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>>>     ^~
>>> sslKeyExchange.c:631:8: note: ...suggests it is guarded by the 'if' clause, but it's not
>>>         goto fail;
>>>         ^~~~
>>> You can even see how the indentation is wrong in the very error message.
>>
>> which suggests reversing the order of the messages, so that they appear
>> in "source" order.
>>
>> I think this is a big improvement in the readability of the warning.
>
> Using this wording order makes it seem that the problem is with the if
> statement, because we emit a warning about it and then emit "only" a
> note for the misleadingly-indented goto statement.

... on second thought, I may be overthinking the semantic difference
between a warning and a note.  Feel free to disregard my nitpicking.

Re: [PATCH] PR c/69993: improvements to wording of -Wmisleading-indentation

[Manuel López-Ibáñez]

On 02/03/16 04:20, Patrick Palka wrote:
>> Using this wording order makes it seem that the problem is with the if
>> statement, because we emit a warning about it and then emit "only" a
>> note for the misleadingly-indented goto statement.
>
> ... on second thought, I may be overthinking the semantic difference
> between a warning and a note.  Feel free to disregard my nitpicking.

This is because the semantics of "note" are confusing. Currently it serves as a 
stand-alone note and as extra info for a warning/error. There is no way to tell 
the difference between the two except by making sense of the text of the 
diagnostic. This is probably not an issue for humans if the text is clear 
enough, but for automatic tools it is impossible to distinguish the two cases. 
There is also the issue of distinguishing two independent single-line notes 
from a multi-line note.

Ideally, we would have a different type of diagnostic

info: some informative text
note: continued here


Cheers,

	Manuel.

Re: [PATCH] PR c/69993: improvements to wording of -Wmisleading-indentation

[David Malcolm]

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

On Tue, 2016-03-01 at 20:18 +0100, Richard Biener wrote:
> On March 1, 2016 7:51:01 PM GMT+01:00, David Malcolm <
> dmalcolm@redhat.com> wrote:
> > The wording of our output from -Wmisleading-indentation is rather
> > confusing, as noted by Reddit user "sysop073" here:
> > https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmislead
> > ingindentation_vs_goto_fail/d0eonwd
> > 
> > > The way they split up the warning looks designed to trick you.
> > > sslKeyExchange.c:631:8: warning: statement is indented as if it
> > > were
> > guarded by... [-Wmisleading-indentation]
> > >         goto fail;
> > >         ^~~~
> > > sslKeyExchange.c:629:4: note: ...this 'if' clause, but it is not
> > >     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
> > >     ^~
> > > You read the first half and it sounds like goto fail; is guarding
> > something. Why would it not be:
> > > sslKeyExchange.c:631:8: warning: statement is wrongly indented...
> > [-Wmisleading-indentation]
> > >         goto fail;
> > >         ^~~~
> > > sslKeyExchange.c:629:4: note: ...as if it were guarded by this
> > > 'if'
> > clause
> > >     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
> > >     ^~
> > 
> > I agree that the current wording is suboptimal; certainly the
> > wording
> > would be much clearer if the wording of the "warning" only spoke
> > about
> > the
> > statement in question, and the "note"/inform should then talk about
> > the
> > not-really-guarding guard.
> > 
> > One rewording could be:
> > 
> > sslKeyExchange.c:631:8: warning: statement is misleadingly
> > indented...
> > [-Wmisleading-indentation]
> >        goto fail;
> >        ^~~~
> > sslKeyExchange.c:629:4: note: ...as if it were guarded by this 'if'
> > clause, but it is not
> >    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
> >    ^~
> > 
> > However another Reddit user ("ksion") noted here:
> > https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmislead
> > ingindentation_vs_goto_fail/d0eqyih
> > that:
> > > This is just passive voice, there is nothing tricky about it.
> > > What I find more confusing -- and what your fix preserves -- is
> > > the
> > > reversed order of offending lines of code in the source file and
> > > the
> > message.
> > > 
> > > I'd rather go with something like this:
> > > sslKeyExchange.c:629:4: warning: indentation of a statement below
> > this 'if' clause... [-Wmisleading-indentation]
> > >     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
> > >     ^~
> > > sslKeyExchange.c:631:8: note: ...suggests it is guarded by the
> > > 'if'
> > clause, but it's not
> > >         goto fail;
> > >         ^~~~
> > > You can even see how the indentation is wrong in the very error
> > message.
> > 
> > which suggests reversing the order of the messages, so that they
> > appear
> > in "source" order.
> > 
> > I think this is a big improvement in the readability of the
> > warning.
> > 
> > The attached patch implements such a change, so that the warning is
> > issued on the supposed guard clause, followed by the note on the
> > statement that isn't really guarded.
> > 
> > Some examples:
> > 
> > Wmisleading-indentation-3.c:18:3: warning: this 'for' clause does
> > not
> > guard... [-Wmisleading-indentation]
> >   for (i = 0; i < 10; i++)
> >   ^~~
> > Wmisleading-indentation-3.c:20:5: note: ...this statement, but the
> > latter is indented as if it does
> >     prod[i] = a[i] * b[i];
> >     ^~~~
> > Wmisleading-indentation-3.c: In function 'fn_6':
> > Wmisleading-indentation-3.c:39:2: warning: this 'if' clause does
> > not
> > guard... [-Wmisleading-indentation]
> >  if ((err = foo (b)) != 0)
> >  ^~
> > Wmisleading-indentation-3.c:41:3: note: ...this statement, but the
> > latter is indented as if it does
> >   goto fail;
> >   ^~~~
> > 
> > I'm not totally convinced by my new wording; maybe the note could
> > also mention the kind of clause ('if'/'while'/'else'/'for') for
> > clarity, maybe something like:
> > 
> > Wmisleading-indentation-3.c: In function 'fn_6':
> > Wmisleading-indentation-3.c:39:2: warning: this 'if' clause does
> > not
> > guard... [-Wmisleading-indentation]
> >  if ((err = foo (b)) != 0)
> >  ^~
> > Wmisleading-indentation-3.c:41:3: note: ...this statement, but the
> > latter is misleadingly indented
> > as if it is guarded by the 'if'
> >   goto fail;
> >   ^~~~
> > 
> > Also, it's slightly clunkier when it comes to macros, e.g.:
> > 
> > Wmisleading-indentation-3.c: In function 'fn_14':
> > Wmisleading-indentation-3.c:60:3: warning: this 'for' clause does
> > not
> > guard... [-Wmisleading-indentation]
> >   for ((VAR) = (START); (VAR) < (STOP); (VAR++))
> >   ^
> > Wmisleading-indentation-3.c:65:3: note: in expansion of macro
> > 'FOR_EACH'
> >   FOR_EACH (i, 0, 10)
> >   ^~~~~~~~
> > Wmisleading-indentation-3.c:67:5: note: ...this statement, but the
> > latter is indented as if it does
> >     bar (i, i);
> >     ^~~
> > 
> > That said, the reordering idea is something I'd like to do for GCC
> > 6.
> > Failing that, there's the tweak to the wording suggested at the
> > top.
> > 
> > OK for trunk? (assuming we can agree on the wording, and that the
> > latest
> > version passes testing)
> 
> OK if others don't disagree.

Thanks.

I took the liberty of tweaking the wording to the longer one given
above, which repeats the type of the clause (for clarity), refreshed it
to cover new tests for -Wmisleading-indentation, verified bootstrap and
regrtest, and committed it as r234403 (attached, for reference).

Here's what the new wording looks like on CVE-2014-1266:

sslKeyExchange.c: In function ‘SSLVerifySignedServerKeyExchange’:
sslKeyExchange.c:629:3: warning: this ‘if’ clause does not guard... [
-Wmisleading-indentation]
   if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
   ^~
sslKeyExchange.c:631:5: note: ...this statement, but the latter is
misleadingly indented as if it is guarded by the ‘if’
     goto fail;
     ^~~~

I plan to update the website accordingly.

Dave

[-- Attachment #2: r234403.patch --]
[-- Type: text/x-patch, Size: 24856 bytes --]

Index: gcc/c-family/c-indentation.c
===================================================================
--- gcc/c-family/c-indentation.c	(revision 234402)
+++ gcc/c-family/c-indentation.c	(revision 234403)
@@ -602,10 +602,12 @@
 					      body_tinfo,
 					      next_tinfo))
     {
-      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",
+      if (warning_at (guard_tinfo.location, OPT_Wmisleading_indentation,
+		      "this %qs clause does not guard...",
+		      guard_tinfo_to_string (guard_tinfo)))
+	inform (next_tinfo.location,
+		("...this statement, but the latter is misleadingly indented"
+		 " as if it is guarded by the %qs"),
 		guard_tinfo_to_string (guard_tinfo));
     }
 }
Index: gcc/c-family/ChangeLog
===================================================================
--- gcc/c-family/ChangeLog	(revision 234402)
+++ gcc/c-family/ChangeLog	(revision 234403)
@@ -1,3 +1,10 @@
+2016-03-22  David Malcolm  <dmalcolm@redhat.com>
+
+	PR c/69993
+	* c-indentation.c (warn_for_misleading_indentation): Rewrite the
+	diagnostic text, reversing the order of the warning and note so
+	that they appear in source order.
+
 2016-03-17  Marek Polacek  <polacek@redhat.com>
 
 	PR c/69407
Index: gcc/testsuite/gcc.dg/plugin/location-overflow-test-2.c
===================================================================
--- gcc/testsuite/gcc.dg/plugin/location-overflow-test-2.c	(revision 234402)
+++ gcc/testsuite/gcc.dg/plugin/location-overflow-test-2.c	(revision 234403)
@@ -20,7 +20,7 @@
 fn_1 (int flag)
 {
   int foo = 4, bar = 5;
-  if (flag) foo = 3; bar = 2; /* { dg-warning "indented" } */
+  if (flag) foo = 3; bar = 2; /* { dg-warning "this .if." } */
   return foo * bar;
 }
 
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog	(revision 234402)
+++ gcc/testsuite/ChangeLog	(revision 234403)
@@ -1,3 +1,12 @@
+2016-03-22  David Malcolm  <dmalcolm@redhat.com>
+
+	PR c/69993
+	* c-c++-common/Wmisleading-indentation-3.c: New test, based on
+	Wmisleading-indentation.c.
+	* c-c++-common/Wmisleading-indentation.c: Update thoughout to
+	reflect change to diagnostic text and order of messages.
+	* gcc.dg/plugin/location-overflow-test-2.c: Likewise.
+
 2016-03-22  David Edelsohn  <dje.gcc@gmail.com>
 
 	* g++.dg/ext/java-3.C: Don't compile on AIX.
Index: gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c
===================================================================
--- gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c	(revision 0)
+++ gcc/testsuite/c-c++-common/Wmisleading-indentation-3.c	(revision 234403)
@@ -0,0 +1,82 @@
+/* Verify -Wmisleading-indentation with source-printing.
+   This is a subset of Wmisleading-indentation.c.  */
+
+/* { dg-options "-Wmisleading-indentation -fdiagnostics-show-caret" } */
+/* { dg-do compile } */
+
+extern int foo (int);
+extern int bar (int, int);
+extern int flagA;
+extern int flagB;
+extern int flagC;
+extern int flagD;
+
+void
+fn_5 (double *a, double *b, double *sum, double *prod)
+{
+  int i = 0;
+  for (i = 0; i < 10; i++) /* { dg-warning "3: this 'for' clause does not guard..." } */
+    sum[i] = a[i] * b[i];
+    prod[i] = a[i] * b[i]; /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'for'" } */
+/* { dg-begin-multiline-output "" }
+   for (i = 0; i < 10; i++)
+   ^~~
+   { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+     prod[i] = a[i] * b[i];
+     ^~~~
+   { dg-end-multiline-output "" } */
+}
+
+/* Based on CVE-2014-1266 aka "goto fail" */
+int fn_6 (int a, int b, int c)
+{
+	int err;
+
+	/* ... */
+	if ((err = foo (a)) != 0)
+		goto fail;
+	if ((err = foo (b)) != 0) /* { dg-message "2: this 'if' clause does not guard..." } */
+		goto fail;
+		goto fail; /* { dg-message "3: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
+	if ((err = foo (c)) != 0)
+		goto fail;
+	/* ... */
+
+/* { dg-begin-multiline-output "" }
+  if ((err = foo (b)) != 0)
+  ^~
+   { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+   goto fail;
+   ^~~~
+   { dg-end-multiline-output "" } */
+
+fail:
+	return err;
+}
+
+#define FOR_EACH(VAR, START, STOP) \
+  for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-warning "3: this 'for' clause does not guard..." } */
+
+void fn_14 (void)
+{
+  int i;
+  FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
+    foo (i);
+    bar (i, i); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'for'" } */
+
+/* { dg-begin-multiline-output "" }
+   for ((VAR) = (START); (VAR) < (STOP); (VAR++))
+   ^
+   { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+   FOR_EACH (i, 0, 10)
+   ^~~~~~~~
+   { dg-end-multiline-output "" } */
+/* { dg-begin-multiline-output "" }
+     bar (i, i);
+     ^~~
+   { dg-end-multiline-output "" } */
+}
+#undef FOR_EACH
Index: gcc/testsuite/c-c++-common/Wmisleading-indentation.c
===================================================================
--- gcc/testsuite/c-c++-common/Wmisleading-indentation.c	(revision 234402)
+++ gcc/testsuite/c-c++-common/Wmisleading-indentation.c	(revision 234403)
@@ -12,9 +12,9 @@
 fn_1 (int flag)
 {
   int x = 4, y = 5;
-  if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" } */
+  if (flag) /* { dg-warning "3: this 'if' clause does not guard..." } */
     x = 3;
-    y = 2; /* { dg-warning "statement is indented as if it were guarded by..." } */
+    y = 2; /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
   return x * y;
 }
 
@@ -21,8 +21,8 @@
 int
 fn_2 (int flag, int x, int y)
 {
-  if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" } */
-    x++; y++; /* { dg-warning "statement is indented as if it were guarded by..." } */
+  if (flag) /* { dg-warning "3: this 'if' clause does not guard..." } */
+    x++; y++; /* { dg-message "10: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
 
   return x * y;
 }
@@ -33,9 +33,9 @@
   int x = 4, y = 5;
   if (flag)
     x = 3;
-  else /* { dg-message "3: ...this 'else' clause, but it is not" } */
+  else /* { dg-warning "3: this 'else' clause does not guard..." } */
     x = 2;
-    y = 2; /* { dg-warning "statement is indented as if it were guarded by..." } */
+    y = 2; /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'else'" } */
   return x * y;
 }
 
@@ -43,9 +43,9 @@
 fn_4 (double *a, double *b, double *c)
 {
   int i = 0;
-  while (i < 10) /* { dg-message "3: ...this 'while' clause, but it is not" } */
+  while (i < 10) /* { dg-warning "3: this 'while' clause does not guard..." } */
     a[i] = b[i] * c[i];
-    i++; /* { dg-warning "statement is indented as if it were guarded by..." } */
+    i++; /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'while'" } */
 }
 
 void
@@ -52,9 +52,9 @@
 fn_5 (double *a, double *b, double *sum, double *prod)
 {
   int i = 0;
-  for (i = 0; i < 10; i++) /* { dg-output "3: ...this 'for' clause, but it is not" } */
+  for (i = 0; i < 10; i++) /* { dg-warning "3: this 'for' clause does not guard..." } */
     sum[i] = a[i] * b[i];
-    prod[i] = a[i] * b[i]; /* { dg-warning "statement is indented as if it were guarded by..." } */
+    prod[i] = a[i] * b[i]; /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'for'" } */
 }
 
 /* Based on CVE-2014-1266 aka "goto fail" */
@@ -65,9 +65,9 @@
 	/* ... */
 	if ((err = foo (a)) != 0)
 		goto fail;
-	if ((err = foo (b)) != 0) /* { dg-message "2: ...this 'if' clause, but it is not" } */
+	if ((err = foo (b)) != 0) /* { dg-message "2: this 'if' clause does not guard..." } */
 		goto fail;
-		goto fail; /* { dg-warning "statement is indented as if it were guarded by..." } */
+		goto fail; /* { dg-message "3: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
 	if ((err = foo (c)) != 0)
 		goto fail;
 	/* ... */
@@ -80,8 +80,8 @@
 {
   if (bar (p, q))
     {
-      if (p) /* { dg-message "7: ...this 'if' clause, but it is not" } */
-        q++; r++; /* { dg-warning "statement is indented as if it were guarded by..." } */
+      if (p) /* { dg-message "7: this 'if' clause does not guard..." } */
+        q++; r++; /* { dg-message "14: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
       t++;
     }
   return p + q + r + s + t;
@@ -95,20 +95,20 @@
 
 void fn_9 (int flag)
 {
-  if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" } */
+  if (flag) /* { dg-warning "3: this 'if' clause does not guard..." } */
     foo (0);
-    foo (1); /* { dg-warning "statement is indented as if it were guarded by..." } */
+    foo (1); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
 }
 
 void fn_10 (int flag)
 {
-  if (flag) /* { dg-message "3: ...this 'if' clause, but it is not" } */
+  if (flag) /* { dg-warning "3: this 'if' clause does not guard..." } */
     if (flag / 2)
       {
         foo (0);
         foo (1);
       }
-    foo (2); /* { dg-warning "statement is indented as if it were guarded by..." } */
+    foo (2); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
   foo (3);
 }
 
@@ -116,48 +116,48 @@
 {
   if (flagA)
     if (flagB)
-      if (flagC) /* { dg-message "7: ...this 'if' clause, but it is not" } */
+      if (flagC) /* { dg-message "7: this 'if' clause does not guard..." } */
         foo (0);
-        bar (1, 2); /* { dg-warning "statement is indented as if it were guarded by..." } */
+        bar (1, 2); /* { dg-message "9: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
 }
 
 void fn_12 (void)
 {
   if (flagA)
-    if (flagB) /* { dg-message "5: ...this 'if' clause, but it is not" } */
+    if (flagB) /* { dg-message "5: this 'if' clause does not guard..." } */
       if (flagC)
         foo (0);
-      bar (1, 2); /* { dg-warning "statement is indented as if it were guarded by..." } */
+      bar (1, 2); /* { dg-message "7: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
 }
 
 void fn_13 (void)
 {
-  if (flagA) /* { dg-message "3: ...this 'if' clause, but it is not" } */
+  if (flagA) /* { dg-warning "3: this 'if' clause does not guard..." } */
     if (flagB)
       if (flagC)
         foo (0);
-    bar (1, 2); /* { dg-warning "statement is indented as if it were guarded by..." } */
+    bar (1, 2); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
 }
 
 #define FOR_EACH(VAR, START, STOP) \
-  for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-message "3: ...this 'for' clause, but it is not" } */
+  for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-warning "3: this 'for' clause does not guard..." } */
 
 void fn_14 (void)
 {
   int i;
-  FOR_EACH (i, 0, 10) /* { dg-message "3: in expansion of macro" } */
+  FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
     foo (i);
-    bar (i, i); /* { dg-warning "statement is indented as if it were guarded by..." } */
+    bar (i, i); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'for'" } */
 }
 #undef FOR_EACH
 
-#define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-message "36: ...this 'for' clause, but it is not" } */
+#define FOR_EACH(VAR, START, STOP) for ((VAR) = (START); (VAR) < (STOP); (VAR++)) /* { dg-message "36: this 'for' clause does not guard..." } */
 void fn_15 (void)
 {
   int i;
-  FOR_EACH (i, 0, 10) /* { dg-message "3: in expansion of macro" } */
+  FOR_EACH (i, 0, 10) /* { dg-message "in expansion of macro .FOR_EACH." } */
     foo (i);
-    bar (i, i); /* { dg-warning "statement is indented as if it were guarded by..." } */
+    bar (i, i); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'for'" } */
 }
 #undef FOR_EACH
 
@@ -166,9 +166,9 @@
   int i;
   for (i = 0; i < 10; i++)
     while (flagA)
-      if (flagB) /* { dg-message "7: ...this 'if' clause, but it is not" } */
+      if (flagB) /* { dg-message "7: this 'if' clause does not guard..." } */
         foo (0);
-        foo (1); /* { dg-warning "statement is indented as if it were guarded by..." } */
+        foo (1); /* { dg-message "9: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
 }
 
 void fn_16_tabs (void)
@@ -176,29 +176,29 @@
   int i;
   for (i = 0; i < 10; i++)
     while (flagA)
-      if (flagB) /* { dg-message "7: ...this 'if' clause, but it is not" } */
+      if (flagB) /* { dg-message "7: this 'if' clause does not guard..." } */
 	foo (0);
-	foo (1);/* { dg-warning "statement is indented as if it were guarded by..." } */
+	foo (1);/* { dg-message "2: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
 }
 
 void fn_17_spaces (void)
 {
   int i;
-  for (i = 0; i < 10; i++) /* { dg-message "3: ...this 'for' clause, but it is not" } */
+  for (i = 0; i < 10; i++) /* { dg-warning "3: this 'for' clause does not guard..." } */
     while (flagA)
       if (flagB)
         foo (0);
-    foo (1);/* { dg-warning "statement is indented as if it were guarded by..." } */
+    foo (1);/* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'for'" } */
 }
 
 void fn_17_tabs (void)
 {
   int i;
-  for (i = 0; i < 10; i++) /* { dg-message "3: ...this 'for' clause, but it is not" } */
+  for (i = 0; i < 10; i++) /* { dg-warning "3: this 'for' clause does not guard..." } */
     while (flagA)
       if (flagB)
 	foo (0);
-    foo (1);/* { dg-warning "statement is indented as if it were guarded by..." } */
+    foo (1);/* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'for'" } */
 }
 
 void fn_18_spaces (void)
@@ -205,10 +205,10 @@
 {
   int i;
   for (i = 0; i < 10; i++)
-    while (flagA) /* { dg-message "5: ...this 'while' clause, but it is not" } */
+    while (flagA) /* { dg-message "5: this 'while' clause does not guard..." } */
       if (flagB)
         foo (0);
-      foo (1);/* { dg-warning "statement is indented as if it were guarded by..." } */
+      foo (1);/* { dg-message "7: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'while'" } */
 }
 
 void fn_18_tabs (void)
@@ -215,10 +215,10 @@
 {
   int i;
   for (i = 0; i < 10; i++)
-    while (flagA) /* { dg-message "5: ...this 'while' clause, but it is not" } */
+    while (flagA) /* { dg-message "5: this 'while' clause does not guard..." } */
       if (flagB)
 	foo (0);
-      foo (1);/* { dg-warning "statement is indented as if it were guarded by..." } */
+      foo (1);/* { dg-message "7: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'while'" } */
 }
 
 /* This shouldn't lead to a warning.  */
@@ -701,40 +701,40 @@
   int i;
 
 #define EMPTY
-#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP; VAR++)
+#define FOR_EACH(VAR, START, STOP) for (VAR = START; VAR < STOP; VAR++) /* { dg-warning "this 'for' clause" } */
 
-  while (flagA); /* { dg-message "3: ...this 'while' clause" } */
-    foo (0); /* { dg-warning "statement is indented as if" } */
+  while (flagA); /* { dg-warning "3: this 'while' clause" } */
+    foo (0); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'while'" } */
 
   if (flagA)
     ;
-  else if (flagB); /* { dg-message "8: ...this 'if' clause" } */
-    foo (0); /* { dg-warning "statement is indented as if" } */
-  while (flagA) /* { dg-message "3: ...this 'while' clause" } */
+  else if (flagB); /* { dg-warning "8: this 'if' clause" } */
+    foo (0); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
+  while (flagA) /* { dg-warning "3: this 'while' clause" } */
     /* blah */;
-    foo (0); /* { dg-warning "statement is indented as if" } */
+    foo (0); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'while'" } */
 
   if (flagA)
     ;
-  else if (flagB) /* { dg-message "8: ...this 'if' clause" } */
+  else if (flagB) /* { dg-warning "8: this 'if' clause" } */
     foo (1);
-    foo (2); /* { dg-warning "statement is indented as if" } */
+    foo (2); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
 
   if (flagA)
     foo (1);
-  else if (flagB) /* { dg-message "8: ...this 'if' clause" } */
+  else if (flagB) /* { dg-warning "8: this 'if' clause" } */
     foo (2);
-    foo (3); /* { dg-warning "statement is indented as if" } */
+    foo (3); /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
 
-  if (flagB) /* { dg-message "3: ...this 'if' clause" } */
+  if (flagB) /* { dg-warning "3: this 'if' clause" } */
     /* blah */;
-    { /* { dg-warning "statement is indented as if" } */
+    { /* { dg-message "5: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
       foo (0);
     }
 
-  if (flagB) /* { dg-message "3: ...this 'if' clause" } */
+  if (flagB) /* { dg-warning "3: this 'if' clause" } */
     /* blah */;
-   { /* { dg-warning "statement is indented as if" } */
+   { /* { dg-message "4: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'" } */
      foo (0);
    }
 
@@ -741,68 +741,68 @@
 
   if (flagB)
     ;
-  else; foo (0); /* { dg-warning "statement is indented as if" } */
+  else; foo (0); /* { dg-warning "3: this 'else' clause" } */
 
-  if (flagC); foo (2); /* { dg-warning "statement is indented as if" } */
+  if (flagC); foo (2); /* { dg-warning "3: this 'if' clause" } */
 
-  if (flagA)
-    ; /* blah */ { /* { dg-warning "statement is indented as if" } */
+  if (flagA) /* { dg-warning "3: this 'if' clause" } */
+    ; /* blah */ { /* { dg-message "18: ...this statement" } */
       foo (1);
     }
 
-  if (flagB) ; /* { dg-message "3: ...this 'if' clause" } */
-    return; /* { dg-warning "statement is indented as if" } */
+  if (flagB) ; /* { dg-warning "3: this 'if' clause" } */
+    return; /* { dg-message "5: ...this statement" } */
 
-  if (flagB) EMPTY; /* { dg-message "3: ...this 'if' clause" } */
-    foo (1); /* { dg-warning "statement is indented as if" } */
+  if (flagB) EMPTY; /* { dg-warning "3: this 'if' clause" } */
+    foo (1); /* { dg-message "5: ...this statement" } */
 
-  for (i = 0; i < 10; i++); /* { dg-message "3: ...this 'for' clause" } */
-    foo (2); /* { dg-warning "statement is indented as if" } */
+  for (i = 0; i < 10; i++); /* { dg-warning "3: this 'for' clause" } */
+    foo (2); /* { dg-message "5: ...this statement" } */
 
-  FOR_EACH (i, 0, 10);
-    foo (2); /* { dg-warning "statement is indented as if" } */
+  FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
+    foo (2); /* { dg-message "5: ...this statement" } */
 
-  FOR_EACH (i, 0, 10);
-    { /* { dg-warning "statement is indented as if" } */
+  FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
+    { /* { dg-message "5: ...this statement" } */
       foo (3);
     }
 
-  FOR_EACH (i, 0, 10);
-  { /* { dg-warning "statement is indented as if" } */
+  FOR_EACH (i, 0, 10); /* { dg-message "3: in expansion of macro .FOR_EACH." } */
+  { /* { dg-message "3: ...this statement" } */
     foo (3);
   }
 
-  while (i++); { /* { dg-warning "statement is indented as if" } */
+  while (i++); { /* { dg-warning "3: this 'while' clause" } */
     foo (3);
   }
 
-  if (i++); { /* { dg-warning "statement is indented as if" } */
+  if (i++); { /* { dg-warning "3: this 'if' clause" } */
     foo (3);
   }
 
   if (flagA) {
     foo (1);
-  } else /* { dg-message "5: ...this 'else' clause" } */
+  } else /* { dg-warning "5: this 'else' clause" } */
     if (flagB)
        foo (2);
-    foo (3); /* { dg-warning "statement is indented as if" } */
+    foo (3); /* { dg-message "5: ...this statement" } */
 
   if (flagA)
     foo (1);
-  else if (flagB); /* { dg-message "8: ...this 'if' clause" } */
-    foo (2); /* { dg-warning "statement is indented as if" } */
+  else if (flagB); /* { dg-warning "8: this 'if' clause" } */
+    foo (2); /* { dg-message "5: ...this statement" } */
 
-  for (i = 0; /* { dg-message "3: ...this 'for' clause" } */
+  for (i = 0; /* { dg-warning "3: this 'for' clause" } */
        i < 10;
        i++);
-    foo (i); /* { dg-warning "statement is indented as if" } */
+    foo (i); /* { dg-message "5: ...this statement" } */
 
   if (flagA)
   {
     foo (1);
   }
-  else if (flagB); /* { dg-message "8: ...this 'if' clause" } */
-  { /* { dg-warning "statement is indented as if" } */
+  else if (flagB); /* { dg-warning "8: this 'if' clause" } */
+  { /* { dg-message "3: ...this statement" } */
     foo (2);
   }
 
@@ -1025,10 +1025,10 @@
 
     if (locked)
         i = foo (0);
-    else /* { dg-message "...this .else. clause" } */
+    else /* { dg-warning "this .else. clause" } */
         i = foo (1);
         engine_ref_debug(e, 0, -1)
-        if (i > 0) /* { dg-warning "statement is indented" } */
+        if (i > 0) /* { dg-message "...this statement" } */
         return 1;
     return 0;
 #undef engine_ref_debug
@@ -1117,7 +1117,7 @@
     foo (1);
   } else if (flagB) /* { dg-message "...this .if. clause" } */
    foo (2);
-   foo (3); /* { dg-warning "statement is indented" } */
+   foo (3); /* { dg-message "...this statement" } */
 }
 
 /* Aligned with the "else".  Likewise, we should warn.  */
@@ -1129,7 +1129,7 @@
     foo (1);
   } else if (flagB) /* { dg-message "...this .if. clause" } */
     foo (2);
-    foo (3); /* { dg-warning "statement is indented" } */
+    foo (3); /* { dg-message "...this statement" } */
 }
 
 /* Indented between the "else" and the "if".  Likewise, we should warn.  */
@@ -1141,7 +1141,7 @@
     foo (1);
   } else if (flagB) /* { dg-message "...this .if. clause" } */
       foo (2);
-      foo (3); /* { dg-warning "statement is indented" } */
+      foo (3); /* { dg-message "...this statement" } */
 }
 
 /* Aligned with the "if".  Likewise, we should warn.  */
@@ -1151,9 +1151,9 @@
 {
   if (flagA) {
     foo (1);
-  } else if (flagB) /* { dg-message "...this .else. clause" } */
+  } else if (flagB) /* { dg-warning "this .else. clause" } */
          foo (2);
-         foo (3); /* { dg-warning "statement is indented" } */
+         foo (3); /* { dg-message "...this statement" } */
 }
 
 /* Indented more than the "if".  Likewise, we should warn.  */
@@ -1165,7 +1165,7 @@
     foo (1);
   } else if (flagB) /* { dg-message "...this .if. clause" } */
             foo (2);
-            foo (3); /* { dg-warning "statement is indented" } */
+            foo (3); /* { dg-message "...this statement" } */
 }
 
 /* Again, but without the 2nd "if".  */
@@ -1210,9 +1210,9 @@
 {
   if (flagA) {
     foo (1);
-  } else  /* { dg-message "...this .else. clause" } */
+  } else  /* { dg-warning "this .else. clause" } */
    foo (2);
-   foo (3);  /* { dg-warning "statement is indented" } */
+   foo (3);  /* { dg-message "...this statement" } */
 }
 
 /* Aligned with the "else".  Likewise, we should warn.  */
@@ -1222,9 +1222,9 @@
 {
   if (flagA) {
     foo (1);
-  } else  /* { dg-message "...this .else. clause" } */
+  } else  /* { dg-warning "this .else. clause" } */
     foo (2);
-    foo (3);  /* { dg-warning "statement is indented" } */
+    foo (3);  /* { dg-message "...this statement" } */
 }
 
 /* Indented more than the "else".  Likewise, we should warn.  */
@@ -1234,7 +1234,7 @@
 {
   if (flagA) {
     foo (1);
-  } else  /* { dg-message "...this .else. clause" } */
+  } else  /* { dg-warning "this .else. clause" } */
         foo (2);
-        foo (3);  /* { dg-warning "statement is indented" } */
+        foo (3);  /* { dg-message "...this statement" } */
 }

[committed] [wwwdocs] PR c/69993: update wording of -Wmisleading-indentation on website

[David Malcolm]

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

On Tue, 2016-03-22 at 10:28 -0400, David Malcolm wrote:
> On Tue, 2016-03-01 at 20:18 +0100, Richard Biener wrote:
> > On March 1, 2016 7:51:01 PM GMT+01:00, David Malcolm <
> > dmalcolm@redhat.com> wrote:
> > > The wording of our output from -Wmisleading-indentation is rather
> > > confusing, as noted by Reddit user "sysop073" here:
> > > https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmisle
> > > ad
> > > ingindentation_vs_goto_fail/d0eonwd
> > > 
> > > > The way they split up the warning looks designed to trick you.
> > > > sslKeyExchange.c:631:8: warning: statement is indented as if it
> > > > were
> > > guarded by... [-Wmisleading-indentation]
> > > >         goto fail;
> > > >         ^~~~
> > > > sslKeyExchange.c:629:4: note: ...this 'if' clause, but it is
> > > > not
> > > >     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) !=
> > > > 0)
> > > >     ^~
> > > > You read the first half and it sounds like goto fail; is
> > > > guarding
> > > something. Why would it not be:
> > > > sslKeyExchange.c:631:8: warning: statement is wrongly
> > > > indented...
> > > [-Wmisleading-indentation]
> > > >         goto fail;
> > > >         ^~~~
> > > > sslKeyExchange.c:629:4: note: ...as if it were guarded by this
> > > > 'if'
> > > clause
> > > >     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) !=
> > > > 0)
> > > >     ^~
> > > 
> > > I agree that the current wording is suboptimal; certainly the
> > > wording
> > > would be much clearer if the wording of the "warning" only spoke
> > > about
> > > the
> > > statement in question, and the "note"/inform should then talk
> > > about
> > > the
> > > not-really-guarding guard.
> > > 
> > > One rewording could be:
> > > 
> > > sslKeyExchange.c:631:8: warning: statement is misleadingly
> > > indented...
> > > [-Wmisleading-indentation]
> > >        goto fail;
> > >        ^~~~
> > > sslKeyExchange.c:629:4: note: ...as if it were guarded by this
> > > 'if'
> > > clause, but it is not
> > >    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
> > >    ^~
> > > 
> > > However another Reddit user ("ksion") noted here:
> > > https://www.reddit.com/r/programming/comments/47pejg/gcc_6_wmisle
> > > ad
> > > ingindentation_vs_goto_fail/d0eqyih
> > > that:
> > > > This is just passive voice, there is nothing tricky about it.
> > > > What I find more confusing -- and what your fix preserves -- is
> > > > the
> > > > reversed order of offending lines of code in the source file
> > > > and
> > > > the
> > > message.
> > > > 
> > > > I'd rather go with something like this:
> > > > sslKeyExchange.c:629:4: warning: indentation of a statement
> > > > below
> > > this 'if' clause... [-Wmisleading-indentation]
> > > >     if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) !=
> > > > 0)
> > > >     ^~
> > > > sslKeyExchange.c:631:8: note: ...suggests it is guarded by the
> > > > 'if'
> > > clause, but it's not
> > > >         goto fail;
> > > >         ^~~~
> > > > You can even see how the indentation is wrong in the very error
> > > message.
> > > 
> > > which suggests reversing the order of the messages, so that they
> > > appear
> > > in "source" order.
> > > 
> > > I think this is a big improvement in the readability of the
> > > warning.
> > > 
> > > The attached patch implements such a change, so that the warning
> > > is
> > > issued on the supposed guard clause, followed by the note on the
> > > statement that isn't really guarded.
> > > 
> > > Some examples:
> > > 
> > > Wmisleading-indentation-3.c:18:3: warning: this 'for' clause does
> > > not
> > > guard... [-Wmisleading-indentation]
> > >   for (i = 0; i < 10; i++)
> > >   ^~~
> > > Wmisleading-indentation-3.c:20:5: note: ...this statement, but
> > > the
> > > latter is indented as if it does
> > >     prod[i] = a[i] * b[i];
> > >     ^~~~
> > > Wmisleading-indentation-3.c: In function 'fn_6':
> > > Wmisleading-indentation-3.c:39:2: warning: this 'if' clause does
> > > not
> > > guard... [-Wmisleading-indentation]
> > >  if ((err = foo (b)) != 0)
> > >  ^~
> > > Wmisleading-indentation-3.c:41:3: note: ...this statement, but
> > > the
> > > latter is indented as if it does
> > >   goto fail;
> > >   ^~~~
> > > 
> > > I'm not totally convinced by my new wording; maybe the note could
> > > also mention the kind of clause ('if'/'while'/'else'/'for') for
> > > clarity, maybe something like:
> > > 
> > > Wmisleading-indentation-3.c: In function 'fn_6':
> > > Wmisleading-indentation-3.c:39:2: warning: this 'if' clause does
> > > not
> > > guard... [-Wmisleading-indentation]
> > >  if ((err = foo (b)) != 0)
> > >  ^~
> > > Wmisleading-indentation-3.c:41:3: note: ...this statement, but
> > > the
> > > latter is misleadingly indented
> > > as if it is guarded by the 'if'
> > >   goto fail;
> > >   ^~~~
> > > 
> > > Also, it's slightly clunkier when it comes to macros, e.g.:
> > > 
> > > Wmisleading-indentation-3.c: In function 'fn_14':
> > > Wmisleading-indentation-3.c:60:3: warning: this 'for' clause does
> > > not
> > > guard... [-Wmisleading-indentation]
> > >   for ((VAR) = (START); (VAR) < (STOP); (VAR++))
> > >   ^
> > > Wmisleading-indentation-3.c:65:3: note: in expansion of macro
> > > 'FOR_EACH'
> > >   FOR_EACH (i, 0, 10)
> > >   ^~~~~~~~
> > > Wmisleading-indentation-3.c:67:5: note: ...this statement, but
> > > the
> > > latter is indented as if it does
> > >     bar (i, i);
> > >     ^~~
> > > 
> > > That said, the reordering idea is something I'd like to do for
> > > GCC
> > > 6.
> > > Failing that, there's the tweak to the wording suggested at the
> > > top.
> > > 
> > > OK for trunk? (assuming we can agree on the wording, and that the
> > > latest
> > > version passes testing)
> > 
> > OK if others don't disagree.
> 
> Thanks.
> 
> I took the liberty of tweaking the wording to the longer one given
> above, which repeats the type of the clause (for clarity), refreshed
> it
> to cover new tests for -Wmisleading-indentation, verified bootstrap
> and
> regrtest, and committed it as r234403 (attached, for reference).
> 
> Here's what the new wording looks like on CVE-2014-1266:
> 
> sslKeyExchange.c: In function ‘SSLVerifySignedServerKeyExchange’:
> sslKeyExchange.c:629:3: warning: this ‘if’ clause does not guard... [
> -Wmisleading-indentation]
>    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
>    ^~
> sslKeyExchange.c:631:5: note: ...this statement, but the latter is
> misleadingly indented as if it is guarded by the ‘if’
>      goto fail;
>      ^~~~
> 
> I plan to update the website accordingly.

I've committed the corresponding change to the website; see the
attached.

Validated; I've also manually doublechecked the built pages on the live
site.

Dave

[-- Attachment #2: update-wording-of-Wmisleading-indentation.patch --]
[-- Type: text/x-patch, Size: 3149 bytes --]

Index: htdocs/gcc-6/changes.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-6/changes.html,v
retrieving revision 1.71
diff -u -p -r1.71 changes.html
--- htdocs/gcc-6/changes.html	14 Mar 2016 10:16:16 -0000	1.71
+++ htdocs/gcc-6/changes.html	22 Mar 2016 15:39:36 -0000
@@ -198,12 +198,12 @@ within strings:
           <a href="https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-1266">CVE-2014-1266</a>:
 <blockquote><pre>
 <b>sslKeyExchange.c:</b> In function <b>'SSLVerifySignedServerKeyExchange'</b>:
-<b>sslKeyExchange.c:631:8:</b> <span class="boldmagenta">warning:</span> statement is indented as if it were guarded by... [<span class="boldmagenta">-Wmisleading-indentation</span>]
-        <span class="boldmagenta">goto</span> fail;
-        <span class="boldmagenta">^~~~</span>
-<b>sslKeyExchange.c:629:4:</b> <span class="boldcyan">note:</span> ...this 'if' clause, but it is not
+<b>sslKeyExchange.c:629:3:</b> <span class="boldmagenta">warning:</span> this <b>'if'</b> clause does not guard... [<span class="boldmagenta">-Wmisleading-indentation</span>]
     <span class="boldcyan">if</span> ((err = SSLHashSHA1.update(&amp;hashCtx, &amp;signedParams)) != 0)
     <span class="boldcyan">^~</span>
+<b>sslKeyExchange.c:631:5:</b> <span class="boldcyan">note:</span> ...this statement, but the latter is misleadingly indented as if it is guarded by the <b>'if'</b>
+        <span class="boldmagenta">goto</span> fail;
+        <span class="boldmagenta">^~~~</span>
 </pre></blockquote>
           This warning is enabled by <code>-Wall</code>.</li>
       </ul>
Index: htdocs/gcc-6/porting_to.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-6/porting_to.html,v
retrieving revision 1.20
diff -u -p -r1.20 porting_to.html
--- htdocs/gcc-6/porting_to.html	2 Mar 2016 21:36:56 -0000	1.20
+++ htdocs/gcc-6/porting_to.html	22 Mar 2016 15:39:36 -0000
@@ -382,12 +382,12 @@ the code might mislead a human reader ab
 
 <blockquote><pre>
 <b>sslKeyExchange.c:</b> In function <b>'SSLVerifySignedServerKeyExchange'</b>:
-<b>sslKeyExchange.c:631:8:</b> <span class="boldmagenta">warning:</span> statement is indented as if it were guarded by... [<span class="boldmagenta">-Wmisleading-indentation</span>]
-        <span class="boldmagenta">goto</span> fail;
-        <span class="boldmagenta">^~~~</span>
-<b>sslKeyExchange.c:629:4:</b> <span class="boldcyan">note:</span> ...this 'if' clause, but it is not
+<b>sslKeyExchange.c:629:3:</b> <span class="boldmagenta">warning:</span> this <b>'if'</b> clause does not guard... [<span class="boldmagenta">-Wmisleading-indentation</span>]
     <span class="boldcyan">if</span> ((err = SSLHashSHA1.update(&amp;hashCtx, &amp;signedParams)) != 0)
     <span class="boldcyan">^~</span>
+<b>sslKeyExchange.c:631:5:</b> <span class="boldcyan">note:</span> ...this statement, but the latter is misleadingly indented as if it is guarded by the <b>'if'</b>
+        <span class="boldmagenta">goto</span> fail;
+        <span class="boldmagenta">^~~~</span>
 </pre></blockquote>
 
 <p>