[PATCH] D106823: [analyzer][solver] Iterate to a fixpoint during symbol simplification with constants
Valeriy Savchenko via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 4 02:17:20 PDT 2021
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
More information about the cfe-commits
mailing list