<br><br><div class="gmail_quote">On Thu, Jun 17, 2010 at 8:54 AM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">On Jun 16, 2010, at 5:37 PM, Jordy Rose wrote:<br>
<br>
> What I'm worried about is that this is going to be overkill for something<br>
> that may never grow beyond additive operations. That is,<br>
> RangeConstraintManager may not be the best way to handle SymSymExprs, or<br>
> even SymIntExprs with other operations in them. (The solution space for<br>
> "x%2 == 0" would have O(MAX) ranges. And a naive transformation for "x/2 >=<br>
> 1" would take the range [1, MAX] and turn it into [2, MAX*2].) I suppose<br>
> right now a Transformer would just be a stack object made of a<br>
> BinaryOperator::Opcode and an APSInt value, while a later one could compose<br>
> operations. Even so, I think it may be a bit of a YAGNI case.<br>
<br>
</div>This is a fair point.  There is some simplicity in the current approach, although it isn't algorithmically scalable.  Perhaps this is the "limit" of "simple" for the the current SimpleConstraintManagers.<br>

<div class="im"><br>
><br>
> Well. I don't think it would be hard to alter the current code to use a<br>
> Transformer -- just change all "X - Adjustment" expressions to<br>
> "T.transform(X)". If you think it's worth it, I can go ahead and do that.<br>
> (My schedule is currently a little spotty cause I'm on vacation, so I may<br>
> not actually have that changed until Sunday.) What it won't do is remove<br>
> any complexity in the disparate AssumeSym*() methods (though allowing<br>
> wraparound ranges would). But it does allow for more possible expansion in<br>
> the future.<br>
<br>
</div>I think we should adopt the change as is, and then refactor with a transformer later if we try to further extend the SimpleConstraintManagers.  It will also give us an opportunity to think about this some more, and understand the design for either a new and more sophisticated ConstraintManager or ways to further extend the current ones.<br>

<br>
Zhongxing: what do you think?</blockquote><div><br>I agree this patch should go in. It's great job. I haven't verified the trick in  RangeConstraintManager::AssumeSymLT. It's not straightforward to get it. But I believe it's right. Maybe some comments are helpful.<br>
</div></div><br>