[PATCH] D103317: [Analyzer][Core] Make SValBuilder to better simplify svals with 3 symbols in the tree

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

To me at least, the patch looks good.
Please post some comparative measurements to demonstrate it won't introduce runtime regression.



================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1144
+                                              : (SVal)SVB.makeIntVal(*Const);
+      return SVal();
+    }
----------------
Let's be explicit about it.


================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1148-1151
+      SVal Ret = getConst(Sym);
+      if (Ret.isUndef())
+        Ret = Visit(Sym);
+      return Ret;
----------------



================
Comment at: clang/test/Analysis/svalbuilder-simplify-compound-svals.cpp:23
+  x = y = z = 1;
+  return 0;
+}
----------------
It feels odd that in some of your examples you return some value, but in the rest, you don't.


================
Comment at: clang/test/Analysis/svalbuilder-simplify-compound-svals.cpp:32
+  clang_analyzer_eval(x + y * z == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(y * z == 0);     // expected-warning{{TRUE}}
+  clang_analyzer_eval(x == 0);         // expected-warning{{TRUE}}
----------------
You could additionally assert that `y == 0` and `z == 0`.


================
Comment at: clang/test/Analysis/svalbuilder-simplify-compound-svals.cpp:50
+void test_SymInt_constrained(int x, int y, int z) {
+  if (x * y * z != 4)
+    return;
----------------
What if `z` were in the middle? Would it still pass?


================
Comment at: clang/test/Analysis/svalbuilder-simplify-compound-svals.cpp:65
+  x = 77 / (y + z);
+  if (y + z != 1)
+    return;
----------------
Would the test pass if you were using `z + y != 1` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103317