[PATCH] D82381: [analyzer] Introduce small improvements to the solver infra

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko marked an inline comment as done.
vsavchenko added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:556
   RangeSet infer(SymbolRef Sym) {
-    const RangeSet *AssociatedRange = State->get<ConstraintRange>(Sym);
-
-    // If Sym is a difference of symbols A - B, then maybe we have range set
-    // stored for B - A.
-    const RangeSet *RangeAssociatedWithNegatedSym =
-        getRangeForMinusSymbol(State, Sym);
-
-    // 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.
-    if (AssociatedRange && RangeAssociatedWithNegatedSym)
-      return AssociatedRange->Intersect(
-          ValueFactory, RangeFactory,
-          RangeAssociatedWithNegatedSym->Negate(ValueFactory, RangeFactory));
-
-    if (AssociatedRange)
-      return *AssociatedRange;
-
-    if (RangeAssociatedWithNegatedSym)
-      return RangeAssociatedWithNegatedSym->Negate(ValueFactory, RangeFactory);
+    if (Optional<RangeSet> ConstraintBasedRange = intersect(
+            ValueFactory, RangeFactory, State->get<ConstraintRange>(Sym),
----------------
NoQ wrote:
> I'm a bit confused, why aren't these invocations of `getRangeForInvertedSub()` and `getRangeForComparisonSymbol()` implemented simply as steps in `Visit()`? What's so different about them, other then our desire not to go into infinite recursion (which should probably be addressed via memoization)?
I even had a patch that moves these two functions inside of the `Visit`, but then I reverted that change.
Right now the logic for each symbol is like this: let's try to find anything on this symbol in the existing constraints and if we did find something - return it.  And only if that didn't work, we try to figure out something from the sub-tree on its own.

So what does that mean in here: that we will need to make a performance impacting [probably nothing serious, but still a functional] change - to intersect the range assigned to the symbol and whatever we can infer from its subtrees.  It would be good to have when we support binary operators `+/-/==/<=/etc.` because it can help narrowing it down.  

As of now, it will give no information, only possible overhead of traversing a tree.  For this reason, I suggest to move those separately and AFTER we support more operators, because that will also have a couple of good motivation examples.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82381