[PATCH] D50256: [Analyzer] [WIP] Basic support for multiplication and division in the constraint manager (for == and != only)

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added a comment.

Thank you for reviewing this patch!



================
Comment at: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:576-597
+  RangeSet New = getRange(St, Sym);
+  for (llvm::APSInt I = AdjustmentType.getZeroValue();
+      I < (Scale.Reciprocal ? AdjustmentType.getValue(1) : Scale.Val); ++I) {
+
+    llvm::APSInt ScInt = AdjInt;
+    if (!rescale(ScInt, Scale, I))
+      continue;
----------------
NoQ wrote:
> I believe that this code should be moved directly into `getRange()`. If it's about looking at a single symbol and figuring out what range information about it do we already have, it should go into `getRange()`. This way we don't need to duplicate it in all the `assume...()` functions, and also it's exactly what `getRange()` is supposed to accomplish.
`getRange()` retrieves the existing range for the symbol. However, similarly to the `Adjustment` we use the `Scale` to change the right side of the relation, not the left one.

I also dislike code multiplication. Maybe we should use the Strategy pattern here and create a function that does the loop. However, if you take a look at D49074 you will see that the body of the loop may look quite different.


================
Comment at: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:616-617
+  RangeSet New = F.getEmptySet();
+  for (llvm::APSInt I = AdjustmentType.getZeroValue();
+      I < (Scale.Reciprocal ? AdjustmentType.getValue(1) : Scale.Val); ++I) {
+  
----------------
NoQ wrote:
> Mmm, what if `Scale.Val` is veeeeeeeery large?
That is a real problem. We either have to limit this functionality for small numbers (for the short term, maybe) or find a better algorithm (for the long term).


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

https://reviews.llvm.org/D50256