Looks great. Thanks so much for doing this! On May 24, 2012, at 7:19 PM, Richard Smith <[email protected]> wrote:
> Hi Ted, > > Thanks for the comments! > > On Thu, May 24, 2012 at 2:33 PM, Ted Kremenek <[email protected]> wrote: > Hi Richard, > > I very much like this work. I have some nit picky comments, but I'd like to > first focus on the high-level ones. > > 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? > > 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. > > 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. > > I've added an example and reworded the comment. > > > int x; > > for (int n = 0; n < size; ++n) { // Redundant "n < size" test. > > // This is somehow guaranteed to happen for some n. > > if (f(n) == k) { > > x = n; > > break; > > } > > } > > return x; > > 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. > > 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. > > More comments in the patch itself: > > All done. > > 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. > > Easily separable cleanups are in r157440, revised patch in r157458. > > Thanks!
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
