[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko requested changes to this revision.
vsavchenko added a comment.
This revision now requires changes to proceed.

Hey, great work!  I think that casts are extremely important, but it looks like you mixed so many things into this patch.  Let's make one step at a time a split it into (at least) a couple of patches.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:757-767
   markDisequal(BasicValueFactory &BV, RangeSet::Factory &F,
-               ProgramStateRef State, SymbolRef First, SymbolRef Second);
+               ProgramStateRef State, NominalTypeList &NTL, SymbolRef First,
+               SymbolRef Second);
   LLVM_NODISCARD static inline ProgramStateRef
   markDisequal(BasicValueFactory &BV, RangeSet::Factory &F,
-               ProgramStateRef State, EquivalenceClass First,
-               EquivalenceClass Second);
+               ProgramStateRef State, NominalTypeList &NTL,
+               EquivalenceClass First, EquivalenceClass Second);
----------------
That's definitely regresses the interface, so `NominalTypeList` should be definitely reworked.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1015-1042
+class NominalTypeList {
+  CanQualType Types[4];
+
+public:
+  using Iterator = CanQualType *;
+
+  NominalTypeList(ASTContext &C)
----------------
This looks like a very `static` data structure to me, I don't see any reasons why the user should be able to create multiple copies of it.
If it becomes a static data-structure, there will be no need in passing it around.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1067-1103
+    do {
+      // We only handle integral cast, when all the types are integrals.
+      // Otherwise, pass the expression to VisitSymExpr.
+      QualType T = RootSym->getType();
+      if (!T->isIntegralOrEnumerationType())
+        return VisitSymExpr(Sym);
+
----------------
I think all this extra logic about how we infer ranges for casts is interesting, but should be a separate patch.
For now, you can simply put `return Visit(Sym->getOperand());`.

First, it will unblock you from depending on that `RangeFactory` feature.
And also have quite a few questions about this particular implementation, so it will stagger this patch as well.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2464-2523
+std::tuple<ProgramStateRef, SymbolRef, RangeSet>
+RangeConstraintManager::handleSymbolCast(ProgramStateRef State, SymbolRef Sym,
+                                         RangeSet R) {
+  QualType T = Sym->getType();
+  if (!T->isIntegralOrEnumerationType() || R.isEmpty())
+    return {State, Sym, R};
+
----------------
I need more explanation why we have this function and why we call it where we call it.  Additionally, it again looks like it belongs in a separate patch.


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

https://reviews.llvm.org/D103096