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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
