<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Looks great. Thanks so much for doing this!<div><br><div><div>On May 24, 2012, at 7:19 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">Hi Ted,<div><br></div><div>Thanks for the comments!<br><br><div class="gmail_quote">On Thu, May 24, 2012 at 2:33 PM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Richard,<br>
<br>
I very much like this work. I have some nit picky comments, but I'd like to first focus on the high-level ones.<br>
<br>
First, I'm wondering if this should just be rolled into -Wuninitialized. If the false positive rate is low (or none), that would seem to be the right thing to do. Otherwise, most users aren't going to benefit from this warning unless they know about it. It seems like the whole point of this patch is to find cases where something is not unconditionally uninitialized but we have much higher confidence in reporting an issue. This patch doesn't try to resolve "maybe uninitialized" issues that could be resolved if we had better path-sensitivity. Thoughts?<br>
</blockquote><div><br></div><div>I think rolling this into -Wuninitialized is best; I've updated the patch to work this way. The old -Wuninitialized is still available via -Wuninitialized -Wno-sometimes-uninitialized, which seems reasonable to me.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Second, I honestly found the description of the algorithm in the comments of getUninitUse() to be (at first) fairly unintuitive. Since the algorithm isn't so obvious, actually having an example in the comments would be great. After I read the code several times, I figured it out, but it also required me working through an example on paper. After that, the comments made sense to me.<br>
</blockquote><div><br></div><div>I've added an example and reworded the comment.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>> int x;<br>
> for (int n = 0; n < size; ++n) { // Redundant "n < size" test.<br>
> // This is somehow guaranteed to happen for some n.<br>
> if (f(n) == k) {<br>
> x = n;<br>
> break;<br>
> }<br>
> }<br>
> return x;<br>
<br>
</div>This is a common source of false positives with the static analyzer, but I'd argue that the code should just initialize 'x' to be safe. I can't look at this in isolation and immediately tell if this is safe, because it requires me making assumptions about size, n, which aren't documented in the code.</blockquote>
<div><br></div><div>Some of the code I found with this pattern had genuine uninitialized-use bugs in it, and the rest of the code seemed clearer when rearranged to avoid the dead condition.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>More comments in the patch itself:</div></blockquote><div><br></div><div>All done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Many of the other changes in the patch seem like just nice cleanups, and not core to this work. I'd prefer they went in separately so that the key algorithmic change is easy to spot in a single (small) patch, but I don't want to waste your time with having to break it down if it is too complicated.</blockquote>
<div><br></div><div>Easily separable cleanups are in r157440, revised patch in r157458.</div><div><br></div><div>Thanks!</div></div></div>
</blockquote></div><br></div></body></html>