[PATCH] D41938: [Analyzer] SValBuilder Comparison Rearrangement (with Restrictions and Analyzer Option)

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.


================
Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:330
+                      nonloc::ConcreteInt(Max), SVB.getConditionType());
+  if (auto DV = IsCappedFromAbove.getAs<DefinedSVal>()) {
+    if (State->assume(*DV, false))
----------------
george.karpenkov wrote:
> 6 lines of branching is probably better expressed as
> 
> ```
> if (!isa<DefinedSVal>(IsCappedFromAbove) || State->assume(*dyn_cast<DefinedSVal>(IsCappedFromAbove), false))
>    return false
> ```
SVal is not a pointer, so isa<>() and dyn_cast<>() does not work here.


================
Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:376
+  // Fail to decompose: "reduce" the problem to the "$x + 0" case.
+  return std::make_tuple(Sym, BO_Add, BV.getValue(0, Sym->getType()));
+}
----------------
george.karpenkov wrote:
> Is it beneficial to do this though? At the point where `decomposeSymbol` is called we are still checking whether the rearrangement could be performed, so maybe just returning a false flag would be better?
Failing to decompose a symbol does not mean the rearrangement could not be performed. If we have just A on the left side instead of A+m or A-m, then we regard it as A+0.


================
Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:432
+    // "$x - $y" vs. "$y - $x" because those are solver's keys.
+    if (LInt > RInt) {
+      ResultSym = SymMgr.getSymSymExpr(RSym, BO_Sub, LSym, SymTy);
----------------
george.karpenkov wrote:
> I think this could be shortened and made more explicit by constructing the LHS and RHS first, and then reversing both and the comparison operator if RHS is negative.
Are you sure it would be shorter? Anyway, how to reverse a SymSymExpr?


https://reviews.llvm.org/D41938