[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:1395
+
+  if (Min > Max) {
+    // This implies that an overflow has occured as either boundary would have
----------------
vsavchenko wrote:
> I commented on this part previously, you shouldn't get fixated on `Min > Max` because `Max >= Min` doesn't imply that overflow didn't happen (less important) and that the range `[Min, Max]` is correct (more important).  Recall one of the examples that I had in that email.
> 
> There is an interesting pattern about results that we can use:
> * no overflows happened -> `[Min, Max]` (should be always true)
> * 1 overflow happened -> we need to invert the range, but there are two different cases:
>     * `Min > Max`, perfect, that's what we expected -> `[Tmin, Max] ∪ [Min, Tmax]`
>     * `Max >= Min`, we overflowed and wrapped the whole range -> `[Tmin, Tmax]`
> * 2 overflows happened on one side -> `[Min, Max]`
> * 2 overflows happened on both sides -> `[Tmin, Tmax]`
> 
> You need to think of this problem in terms of what really happens and not in terms of combating with assertions.
I think I agree with you. This pattern (and the code following this pattern) makes much more sense, and easier to follow. I think I will refactor the patch accordingly. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103440