[cfe-commits] r150306 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp lib/StaticAnalyzer/Checkers/CMakeLists.txt lib/StaticAnalyzer/Checkers/Checkers.td test/Analysis/bool-assignment.cpp test/Analysis/bool-assignment2.c

[Jordy Rose]

On Feb 11, 2012, at 23:32, Ryan Govostes wrote:
> +  // Check if the assigned value meets our criteria for correctness.  It must
> +  // be a value that is either 0 or 1.  One way to check this is to see if
> +  // the value is possibly < 0 (for a negative value) or greater than 1.

Nice idea for a checker. The one thing I'm wondering is why you decided to use this less than / greater than approach...it might not matter here, but in general statements about equality and inequality are a bit easier for the analyzer to reason about than less than and greater than. It requires the same number of checks, too:

stLTZero, stGEZero
stGTOne, stZeroOrOne

or

stZero, stNotZero
stOne, stNotZeroOrOne

Also, in the future it might be worth warning on /any/ nonloc::LocAsInteger that is non-zero, since valid pointers are almost never 1. That would show up, I think, in this Obj-C code:

BOOL result = (BOOL)somePossiblyNilObject;

(Actually, can we just add a compiler warning on /all/ pointer-to-fake-boolean conversions? Truncation will give the wrong answer on 1 in 256 pointers when BOOL is actually a char.)

Finally, the fact that this does /not/ assume that the value is 0 or 1 afterwards is probably a good thing. (Other checkers, like the null dereference checker, add the non-null assumption to an underconstrained state, since the program would be undefined otherwise. The same is true here, but it could probably generate some very confusing errors if, say, there was a switch statement later.)

Jordy