[PATCH] D110357: [Analyzer] Extend ConstraintAssignor to handle remainder op

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

steakhal added a comment.

Great work!



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1591-1595
   template <class ClassOrSymbol>
   LLVM_NODISCARD static ProgramStateRef
-  assign(ProgramStateRef State, SValBuilder &Builder, RangeSet::Factory &F,
-         ClassOrSymbol CoS, RangeSet NewConstraint) {
+  assign(ProgramStateRef State, RangeConstraintManager *RCM,
+         SValBuilder &Builder, RangeSet::Factory &F, ClassOrSymbol CoS,
+         RangeSet NewConstraint) {
----------------
Hm, why don't we acquire `RCM`, `Builder`, `F` in the constructor? I'm expecting all of them to remain the same for all the `assign()` calls.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1603-1604
 
+  template <typename SymT>
+  bool handleRem(const SymT *Sym, RangeSet Constraint) {
+    // a % b != 0 implies that a != 0.
----------------
Why is this not a `const` member function?


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1605
+  bool handleRem(const SymT *Sym, RangeSet Constraint) {
+    // a % b != 0 implies that a != 0.
+    if (Sym->getOpcode() != BO_Rem)
----------------
I think you should also implement `a % b == 0 implies that a == 0`.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1628
 private:
-  ConstraintAssignor(ProgramStateRef State, SValBuilder &Builder,
-                     RangeSet::Factory &F)
-      : State(State), Builder(Builder), RangeFactory(F) {}
+  ConstraintAssignor(ProgramStateRef State, RangeConstraintManager *RCM,
+                     SValBuilder &Builder, RangeSet::Factory &F)
----------------
IMO we should pass a reference here, just like the rest of the parameters.


================
Comment at: clang/test/Analysis/constraint-assignor.c:18
+  clang_analyzer_warnIfReached(); // no-warning
+  (void)x; // keep the constraints alive.
+}
----------------
It's still mindboggling that we need to do this. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110357