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