<div class="gmail_quote">On Mon, Jul 16, 2012 at 5:40 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div><div>On Mon, Jul 16, 2012 at 5:27 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br></div></div><div class="gmail_extra">
<div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div><div>On Mon, Jul 16, 2012 at 5:15 PM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">



<div style="word-wrap:break-word"><div><br><div><div>On Jul 16, 2012, at 17:06 , Richard Smith <<a href="mailto:richard-llvm@metafoo.co.uk" target="_blank">richard-llvm@metafoo.co.uk</a>> wrote:</div><br>
<blockquote type="cite"><span style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;display:inline!important;float:none">int test6() {</span><br style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">



<span style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;display:inline!important;float:none">  int x; // expected-note{{initialize the variable 'x' to silence this warning}}</span><br style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">



<span style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;display:inline!important;float:none">-  x += 2; // expected-warning{{variable 'x' is uninitialized when used here}}</span><br style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">



<span style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;display:inline!important;float:none">-  return x;</span><br style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">



<span style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;display:inline!important;float:none">+  x += 2;</span><br style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">



<span style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;display:inline!important;float:none">+  return x; // expected-warning{{variable 'x' is uninitialized when used here}}</span><br style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">



<span style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;display:inline!important;float:none">}</span><br style="font-family:monospace;font-size:medium;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:-webkit-auto;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">



</blockquote></div><br></div><div>Sorry for not chiming in pre-commit, but I'm not happy with this. The first non-compound-assignment use of a variable could be very far from the variable, and while we do get a note on the decl, it still seems like a QoI problem. I'd rather have compound assignments treated as a use <i>and</i> an initialization.</div>



<div><br></div><div>(Actually, can we just call it a use? Does that hurt anything?)</div></div></blockquote><div><br></div></div></div><div>I mentioned this on the review thread: it introduced "false" positives into my test corpus (FSVO "false"). Basically, if a variable is only ever used in compound-assignments, but its value is never actually used, we have not proven that there is a "real" problem with the code (FSVO "real").</div>



<div><br></div><div>I don't see how the distance between the variable and the use which we report is a problem, since we give diagnostics pointing at both of them. I would prefer to not add this extra checking (which seems to basically be all noise) into -Wuninitialized, but if you feel strongly about this, I'll make the change.</div>


</div></blockquote><div><br></div></div></div><div>While using your condition for triggering the warning at all, could we attach the warning to the 'x += 2', and note the final use? Or at least note the 'x += 2'? That would seem to address most of Jordan's QoI concern w/o increasing false positives.</div>


</div></div>
</blockquote></div><br><div>That doesn't fall naturally out of the algorithm, so I'd be concerned that the added complexity doesn't justify the benefit.</div><div><br></div><div>I'm coming round to Jordy's position. In particular, for "int n; n *= 0; return n;", warning on the return statement does not seem particularly reasonable, and warning on the *= is pedantically correct, since it has undefined behavior. The code samples in which I found the 'false' positives are fairly questionable.</div>