[PATCH] D5102: [analyzer][Bugfix/improvement] Fix for PR16833

[Jordan Rose via cfe-commits]

jordan_rose added a comment.

I guess the regular pings didn't work, so it was worth trying the gentle one? Sorry!

This seems mostly ready to me, but I still have a few comments.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1646-1650
@@ -1644,2 +1645,7 @@
   DefinedOrUnknownSVal CondV = CondV_untested.castAs<DefinedOrUnknownSVal>();
+  if (CondV.isUnknown()) {
+    CondV = state->getStateManager().getSValBuilder().conjureSymbolVal(nullptr,
+          CondE, LCtx, builder.getBuilderContext()->blockCount());
+    state = state->BindExpr(CondE, LCtx, CondV);
+  }
 
----------------
What's the purpose of this? As I understand it, a switch condition value will never be reused in later states, so there's no point in adding constraints to it unless it's already symbolic.

(And not bothering to do this would remove the need to pass the NodeBuilderContext through.)

================
Comment at: lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp:199-200
@@ +198,4 @@
+    // Just add the constraint to the expression without trying to simplify.
+    SymbolRef Sym = Value.getAsSymExpr();
+    return assumeSymWithinInclusiveRange(State, Sym, From, To, InRange);
+  }
----------------
Will this ever return a null symbol? Maybe add an assertion?

================
Comment at: lib/StaticAnalyzer/Core/SimpleConstraintManager.cpp:215-220
@@ +214,8 @@
+      return assumeSymWithinInclusiveRange(State, Sym, From, To, InRange);
+    return State;
+  } // end switch
+
+  case nonloc::ConcreteIntKind: {
+    const llvm::APSInt &IntVal = Value.castAs<nonloc::ConcreteInt>().getValue();
+    bool IsInRange = IntVal >= From && IntVal <= To;
+    bool isFeasible = (IsInRange == InRange);
----------------
This is still relevant.

================
Comment at: test/Analysis/switch-case.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=debug.ExprInspection -verify %s
+
----------------
All tests should include the "core" checkers. At some point we'll probably make that implicit, but for now please add that to the -analyzer-checker list.

================
Comment at: test/Analysis/switch-case.c:24-25
@@ +23,4 @@
+  case 3 ... 10:
+    clang_analyzer_eval(t > 1);        // expected-warning{{TRUE}}
+    clang_analyzer_eval(t + 2 <= 11);  // expected-warning{{TRUE}}
+    clang_analyzer_eval(t + 1 == 3);   // expected-warning{{UNKNOWN}}
----------------
Can you include at least one more check here: `clang_analyzer_eval(t > 2)`? (Which should be unknown.)

================
Comment at: test/Analysis/switch-case.c:122
@@ +121,3 @@
+
+void testDefaultBrachRange(int arg) {
+  switch (arg) {
----------------
Typo: "Brach"


http://reviews.llvm.org/D5102