[PATCH] D103440: [WIP][analyzer] Introduce range-based reasoning for addition operator

[Manas Gupta via Phabricator via cfe-commits]

manas added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1409-1415
+  if ((LHS.From() > 0 && RHS.From() > 0 && Min < 0) ||
+      (LHS.From() < 0 && RHS.From() < 0 && Min > 0) ||
+      (LHS.To() > 0 && RHS.To() > 0 && Max < 0) ||
+      (LHS.To() < 0 && RHS.To() < 0 && Max > 0)) {
+    // return [Tmin, Tmax]
+    return {RangeFactory, Tmin, Tmax};
+  }
----------------
vsavchenko wrote:
> manas wrote:
> > vsavchenko wrote:
> > > manas wrote:
> > > > vsavchenko wrote:
> > > > > I thought we talked quite a lot that there is nothing bad with overflows and here we have that if ANY overflow happened, we bail out and don't give any result.
> > > > Understood! Should I replace it with code returning EmptySet()?
> > > Why `EmptySet()`?  `EmptySet()` means that this can never happen, this path is infeasible.  Is that the case?
> > > Let's say we have: `[INT_MAX - 20, INT_MAX - 10] + [30, 40]` what happens in this case?
> > Right! That will not be the case.
> > 
> > In this particular case, the range will be `[INT_MIN + 9, INT_MIN + 29]` which is far smaller than `[Tmin, Tmax]`.
> > 
> > Also, I think I misunderstood the part of //bailing out and not giving any result// as returning empty.
> Gotcha!
> 
> Because of cases like this, you need to re-think that part about `Min > Max` and maybe count the number of overflows on each side?
That's right. If it is able to deduce how many times overflows are occurring then it can reason about whether it will be MaxRangeSet or a subset of it. Fixing it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103440