On Tue, Mar 1, 2016 at 1:51 PM, David Malcolm <dmalc...@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.
