[PATCH] D73062: [analyzer] Simplify BoolAssignmentChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Thanks!!! Small patches are so nice to review.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp:91
 
-  if (!greaterThanEqualToZero) {
-    // The SValBuilder cannot construct a valid SVal for this condition.
-    // This means we cannot properly reason about it.
-    return;
-  }
-
-  ProgramStateRef stateLT, stateGE;
-  std::tie(stateGE, stateLT) = CM.assumeDual(state, *greaterThanEqualToZero);
-
-  // Is it possible for the value to be less than zero?
-  if (stateLT) {
-    // It is possible for the value to be less than zero.  We only
-    // want to emit a warning, however, if that value is fully constrained.
-    // If it it possible for the value to be >= 0, then essentially the
-    // value is underconstrained and there is nothing left to be done.
-    if (!stateGE)
-      emitReport(stateLT, C);
-
-    // In either case, we are done.
-    return;
-  }
-
-  // If we reach here, it must be the case that the value is constrained
-  // to only be >= 0.
-  assert(stateGE == state);
-
-  // At this point we know that the value is >= 0.
-  // Now check to ensure that the value is <= 1.
-  DefinedSVal OneVal = svalBuilder.makeIntVal(1, valTy);
-  SVal lessThanEqToOneVal =
-    svalBuilder.evalBinOp(state, BO_LE, *DV, OneVal,
-                          svalBuilder.getConditionType());
-
-  Optional<DefinedSVal> lessThanEqToOne =
-      lessThanEqToOneVal.getAs<DefinedSVal>();
-
-  if (!lessThanEqToOne) {
-    // The SValBuilder cannot construct a valid SVal for this condition.
-    // This means we cannot properly reason about it.
-    return;
-  }
-
-  ProgramStateRef stateGT, stateLE;
-  std::tie(stateLE, stateGT) = CM.assumeDual(state, *lessThanEqToOne);
-
-  // Is it possible for the value to be greater than one?
-  if (stateGT) {
-    // It is possible for the value to be greater than one.  We only
-    // want to emit a warning, however, if that value is fully constrained.
-    // If it is possible for the value to be <= 1, then essentially the
-    // value is underconstrained and there is nothing left to be done.
-    if (!stateLE)
-      emitReport(stateGT, C);
-
-    // In either case, we are done.
-    return;
-  }
-
-  // If we reach here, it must be the case that the value is constrained
-  // to only be <= 1.
-  assert(stateLE == state);
+  if (StOut)
+    emitReport(StOut, C);
----------------
I believe this should be `!StIn`. I.e., the right check to do is "the value is known to be not boolean" (this is definitely a buggy execution path the static analyzer managed to discover), whereas "the value is not known to be definitely boolean" is the wrong check to do (the static analyzer doesn't have enough information to conclude that the value is definitely boolean, which might be because the value may be non-boolean, but it's more likely that this happens because the static analyzer simply doesn't know enough about the program).

I think this change should fix the regressions on the test suite.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73062/new/

https://reviews.llvm.org/D73062