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
