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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I guess that's roughly the way to go if we want to expand `RangeConstraintManager` with more math.

One important thing to test here, that doesn't seem to be tested, is what happens when the integer and the scale value are of different types. This may occur because we don't have any representation for integral casts and therefore the type of the symbol itself isn't necessarily the type of the unknown value it represents. As usual, that complicates this sort of work.

I think it's a good idea to organize the constraint manager and `SValBuilder` a bit better, so that it was easier to understand the algorithmic complexity of the newly implemented stuff by looking at small parts of their code.

Also i'd really appreciate if someone refactors `SValBuilder` and/or `RangeConstraintManager` so that it was easier to navigate. The current ad-hoc code structure makes it too hard to figure out what the contract of every function is and how do the changes affect algorithmic complexity of the whole thing. But for this patch it doesn't seem to be that bad.



================
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;
----------------
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.


================
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) {
+  
----------------
Mmm, what if `Scale.Val` is veeeeeeeery large?


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

https://reviews.llvm.org/D50256