[PATCH] D106823: [analyzer][solver] Iterate to a fixpoint during symbol simplification with constants

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Looking great!
I have a couple of nit picks and I kind of want to check that it doesn't affect the performance on a different set of projects as well.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2112-2113
+  assert(ClsMembers.contains(Old));
+  assert(ClsMembers.getHeight() > 1 &&
+         "Class should have at least two members");
+
----------------
Maybe after removing you can check that `ClsMembers` is not empty?
I just don't like relying on `getHeight` because it looks like an implementation detail and shouldn't be used.  We use it only in one place in the whole codebase (in equivalence classes :) ), but since we can re-write this assertion to have a simpler condition, I think that it should be preferred.


================
Comment at: clang/test/Analysis/symbol-simplification-fixpoint-iteration-unreachable-code.cpp:20
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  clang_analyzer_printState();
+
----------------
Do we need to print states in this test?


================
Comment at: clang/test/Analysis/symbol-simplification-fixpoint-one-iteration.cpp:32
+  // CHECK-NEXT:   "equivalence_classes": [
+  // CHECK-NEXT:     [ "(reg_$0<int a>) != (reg_$2<int c>)" ],
+  // CHECK-NEXT:     [ "reg_$0<int a>", "reg_$2<int c>" ]
----------------
Same question here


================
Comment at: clang/test/Analysis/symbol-simplification-fixpoint-two-iterations.cpp:36
+  // CHECK-NEXT:  "equivalence_classes": [
+  // CHECK-NEXT:    [ "(reg_$0<int a>) != (reg_$3<int d>)" ],
+  // CHECK-NEXT:    [ "reg_$0<int a>", "reg_$3<int d>" ],
----------------
OK, I understand the next two classes.
But how did we produce this one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106823