[analyzer][NFC] Tidy up handler-functions in SymbolicRangeInferrer

[ASDenysPetrov]

Summary

Sorted some handler-functions into more appropriate visitor functions of the SymbolicRangeInferrer.

• Spread getRangeForNegatedSub body over several visitor functions: VisitSymExpr, VisitSymIntExpr, VisitSymSymExpr.
• Moved getRangeForComparisonSymbol from infer to VisitSymSymExpr.

[martong]

Took a while to accept that this is indeed an NFC, but looks good! And indeed the code is better structured this way.

[ASDenysPetrov]

Fixed a typo that caused constraint_manager_negate.c and unary-sym-expr.c tests crashes.

[martong]

In D129678#3652859, @ASDenysPetrov wrote:

Fixed a typo that caused constraint_manager_negate.c and unary-sym-expr.c tests crashes.

Good. Still LGTM.

clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

public:
   template <class SourceType>
   static RangeSet inferRange(RangeSet::Factory &F, ProgramStateRef State,
                              SourceType Origin) {
     SymbolicRangeInferrer Inferrer(F, State);
     return Inferrer.infer(Origin);
   }
 
   RangeSet VisitSymExpr(SymbolRef Sym) {
-    // If we got to this function, the actual type of the symbolic
+    if (Optional<RangeSet> RS = getRangeForNegatedSym(Sym))
+      return *RS;
+    // If we've reached this line, the actual type of the symbolic
     // expression is not supported for advanced inference.
     // In this case, we simply backoff to the default "let's simply
     // infer the range from the expression's type".
     return infer(Sym->getType());
   }
 
+  RangeSet VisitUnarySymExpr(const UnarySymExpr *USE) {
+    if (Optional<RangeSet> RS = getRangeForNegatedUnarySym(USE))
+      return *RS;
+    return infer(USE->getType());
+  }
+
   RangeSet VisitSymIntExpr(const SymIntExpr *Sym) {
     return VisitBinaryOperator(Sym);
   }
 
   RangeSet VisitIntSymExpr(const IntSymExpr *Sym) {
     return VisitBinaryOperator(Sym);
   }
 
-  RangeSet VisitSymSymExpr(const SymSymExpr *Sym) {
+  RangeSet VisitSymSymExpr(const SymSymExpr *SSE) {
     return intersect(
         RangeFactory,
+        // If Sym is a difference of symbols A - B, then maybe we have range
+        // set stored for B - A.
+        //
+        // If we have range set stored for both A - B and B - A then
+        // calculate the effective range set by intersecting the range set
+        // for A - B and the negated range set of B - A.
+        getRangeForNegatedSymSym(SSE),
+        // If Sym is a comparison expression (except <=>),
+        // find any other comparisons with the same operands.
+        // See function description.
+        getRangeForComparisonSymbol(SSE),
         // If Sym is (dis)equality, we might have some information
         // on that in our equality classes data structure.
-        getRangeForEqualities(Sym),
+        getRangeForEqualities(SSE),
         // And we should always check what we can get from the operands.
-        VisitBinaryOperator(Sym));
+        VisitBinaryOperator(SSE));
   }
 
 private:
   SymbolicRangeInferrer(RangeSet::Factory &F, ProgramStateRef S)
       : ValueFactory(F.getValueFactory()), RangeFactory(F), State(S) {}
 
   /// Infer range information from the given integer constant.
   ///
if (ActualType->isIntegralOrEnumerationType() ||
       return infer(Sym);
     }
     // Otherwise, let's simply infer from the destination type.
     // We couldn't figure out nothing else about that expression.
     return infer(DestType);
   }
 
   RangeSet infer(SymbolRef Sym) {
-    return intersect(
-        RangeFactory,
-        // Of course, we should take the constraint directly associated with
-        // this symbol into consideration.
+    return intersect(RangeFactory,
+                     // Of course, we should take the constraint directly
+                     // associated with this symbol into consideration.
                      getConstraint(State, Sym),
-        // If Sym is a difference of symbols A - B, then maybe we have range
-        // set stored for B - A.
-        //
-        // If we have range set stored for both A - B and B - A then
-        // calculate the effective range set by intersecting the range set
-        // for A - B and the negated range set of B - A.
-        getRangeForNegatedSub(Sym),
-        // If Sym is a comparison expression (except <=>),
-        // find any other comparisons with the same operands.
-        // See function description.
-        getRangeForComparisonSymbol(Sym),
-        // Apart from the Sym itself, we can infer quite a lot if we look
-        // into subexpressions of Sym.
+                     // Apart from the Sym itself, we can infer quite a lot if
+                     // we look into subexpressions of Sym.
                      Visit(Sym));
   }
 
   RangeSet infer(EquivalenceClass Class) {
     if (const RangeSet *AssociatedConstraint = getConstraint(State, Class))
       return *AssociatedConstraint;
 
     return infer(Class.getType());
   }
private:
   }
 
   /// Return a range set subtracting zero from \p Domain.
   RangeSet assumeNonZero(RangeSet Domain, QualType T) {
     APSIntType IntType = ValueFactory.getAPSIntType(T);
     return RangeFactory.deletePoint(Domain, IntType.getZeroValue());
   }
 
-  Optional<RangeSet> getRangeForNegatedSub(SymbolRef Sym) {
+  template <typename ProduceNegatedSymFunc>
+  Optional<RangeSet> getRangeForNegatedExpr(ProduceNegatedSymFunc F,
+                                            QualType T) {
     // Do not negate if the type cannot be meaningfully negated.
-    if (!Sym->getType()->isUnsignedIntegerOrEnumerationType() &&
-        !Sym->getType()->isSignedIntegerOrEnumerationType())
+    if (!T->isUnsignedIntegerOrEnumerationType() &&
+        !T->isSignedIntegerOrEnumerationType())
       return llvm::None;
 
-    const RangeSet *NegatedRange = nullptr;
-    SymbolManager &SymMgr = State->getSymbolManager();
-    if (const auto *USE = dyn_cast<UnarySymExpr>(Sym)) {
-      if (USE->getOpcode() == UO_Minus) {
-        // Just get the operand when we negate a symbol that is already negated.
-        // -(-a) == a
-        NegatedRange = getConstraint(State, USE->getOperand());
-      }
-    } else if (const SymSymExpr *SSE = dyn_cast<SymSymExpr>(Sym)) {
-      if (SSE->getOpcode() == BO_Sub) {
-        QualType T = Sym->getType();
-        SymbolRef NegatedSym =
-            SymMgr.getSymSymExpr(SSE->getRHS(), BO_Sub, SSE->getLHS(), T);
-        NegatedRange = getConstraint(State, NegatedSym);
-      }
-    } else {
-      SymbolRef NegatedSym =
-          SymMgr.getUnarySymExpr(Sym, UO_Minus, Sym->getType());
-      NegatedRange = getConstraint(State, NegatedSym);
-    }
+    if (SymbolRef NegatedSym = F())
+      if (const RangeSet *NegatedRange = getConstraint(State, NegatedSym))
+        return RangeFactory.negate(*NegatedRange);
+
+    return llvm::None;
+  }
+
+  Optional<RangeSet> getRangeForNegatedUnarySym(const UnarySymExpr *USE) {
+    return getRangeForNegatedExpr(
+        [USE]() -> SymbolRef {
+          if (USE->getOpcode() == UO_Minus)
+            return USE->getOperand();
+          return nullptr;
+        },
+        USE->getType());
+
+  Optional<RangeSet> getRangeForNegatedSymSym(const SymSymExpr *SSE) {
+    return getRangeForNegatedExpr(
+        [SSE, State = this->State]() -> SymbolRef {
+          if (SSE->getOpcode() == BO_Sub)
+            return State->getSymbolManager().getSymSymExpr(
+                SSE->getRHS(), BO_Sub, SSE->getLHS(), SSE->getType());
+          return nullptr;
+        },
+        SSE->getType());
 
-    if (NegatedRange)
-      return RangeFactory.negate(*NegatedRange);
-    return llvm::None;
+  Optional<RangeSet> getRangeForNegatedSym(SymbolRef Sym) {
+    return getRangeForNegatedExpr(
+        [Sym, State = this->State]() {
+          return State->getSymbolManager().getUnarySymExpr(Sym, UO_Minus,
+                                                           Sym->getType());
+        },
+        Sym->getType());
   }
 
   // Returns ranges only for binary comparison operators (except <=>)
   // when left and right operands are symbolic values.
   // Finds any other comparisons with the same operands.
   // Then do logical calculations and refuse impossible branches.
   // E.g. (x < y) and (x > y) at the same time are impossible.
   // E.g. (x >= y) and (x != y) at the same time makes (x > y) true only.
   // E.g. (x == y) and (y == x) are just reversed but the same.
   // It covers all possible combinations (see CmpOpTable description).
   // Note that `x` and `y` can also stand for subexpressions,
   // not only for actual symbols.
-  Optional<RangeSet> getRangeForComparisonSymbol(SymbolRef Sym) {
-    const auto *SSE = dyn_cast<SymSymExpr>(Sym);
-    if (!SSE)
-      return llvm::None;
-
+  Optional<RangeSet> getRangeForComparisonSymbol(const SymSymExpr *SSE) {
     const BinaryOperatorKind CurrentOP = SSE->getOpcode();
 
     // We currently do not support <=> (C++20).
     if (!BinaryOperator::isComparisonOp(CurrentOP) || (CurrentOP == BO_Cmp))
       return llvm::None;
 
     static const OperatorRelationsTable CmpOpTable{};