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

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

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

Commit Message

Comments

Patch