Hi Jordan, see below for comments and questions. < Hi, Anders. I have some high-level concerns about this checker as it's structured now. First (and most mundanely), the checker is conflating values and locations. A particular symbol in the analyzer can <never change, so there's no reason to ever have to remove something from the DivZeroMap because of invalidation. < < int x = foo(); // The value stored in 'x' is some symbol '$foo' < int y = 5 / x; // use $foo < x = bar(); // The value stored in 'x' is now some symbol '$bar', but '$foo' hasn't changed.
< On the flip side, I'm concerned that values never leave the map. You could clean up some of them with a checkDeadSymbols callback, but that's still a large amount of state for values that may have been < checked quite a while ago. In particular, this will result in false positives if a value is used in one function and then checked in another. Why don't we have to remove it? When x changes as you say we need to remove it or flag it so we don't check x against zero anymore since x has changed after the division. I guess we could remove all items in DivZeroMap when we leave the function, do you think that would work? < < (The opposite order hit us so much when dealing with C++ support that we coined a term for it: "inlined defensive checks". This occurs when one function accepts null pointers and so does a check, the <caller knows the value is never null but doesn't actually assert it, and then the value is used. We ended up having to silence all bugs where the first null check came from an inlined function, which <definitely through out some true positives with the false ones.) < < What do you think? < Jordan < < P.S. If your map entries always map to "true", you might as well use a set. ;-) Is it possible to get a specific entry using set or do you have to iterate through all items to find the one your looking for? With map you can do this,: const bool D = State->get<DivZeroMap>(MR); but with set i'm only able to get them all. DivZeroMapTy DivZeroes = State->get<DivZeroMap>(); /Anders On Apr 1, 2014, at 7:45 , Anders Rönnholm <[email protected]<mailto:[email protected]>> wrote: Hi, I have a new patch i'd like to get reviewed for a different kind of division by zero than the already existing one. This patch check for division by variable that is later compared against 0. Either the comparison is useless or there is division by zero. It catches scenarios like these: void f(int y) { x = 100 / y; if (y == 0) {} } //Anders<divzero2.diff>_______________________________________________ cfe-commits mailing list [email protected]<mailto:[email protected]> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
