diff mbox

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

Message ID 1458661582.9902.91.camel@redhat.com
State New
Headers show

Commit Message

David Malcolm March 22, 2016, 3:46 p.m. UTC 
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
diff mbox

Patch

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>