On Tue, Mar 1, 2016 at 3:29 PM, Patrick Palka <patr...@parcs.ath.cx> wrote: > 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.
... on second thought, I may be overthinking the semantic difference between a warning and a note. Feel free to disregard my nitpicking.
