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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
