[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 inline comments.
Herald added a subscriber: jdoerfert.


================
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;
----------------
baloghadamsoftware wrote:
> 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.
I took a look again at all the loops including D49074, but only the loop conditions match. There are no other similarities between the loop bodies. I can move the loop into another function taking a lambda as the loop body but it does not simplify the code so I see no point in it.


================
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) {
+  
----------------
baloghadamsoftware wrote:
> 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).
Any suggestion for the limit? Maybe `256`?


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

https://reviews.llvm.org/D50256