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