[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D103096#2867441 <https://reviews.llvm.org/D103096#2867441>, @ASDenysPetrov wrote:

> @vsavchenko
>
>> Why did you write it this way!?
>
> I want the map contains only valid constraints at any time, so we can easely get them without traversing with all variants intersecting with each other. I'm gonna move `updateExistingConstraints ` logic to `VisitSymbolCast`. I think your suggestion can even improve the feature and cover some more cases. I'll add more tests in the next update. Thanks!

`[-10, 10]` is also valid, right?  You can't keep things at their best all the time.  And if you want all constraints directly in the map then what's all this logic in `VisitSymbolCast`?  That's why I keep asking why do you need both parts of this solution and didn't get any answer so far.
I'm hands down for the incremental approach and adding small-to-medium size improvements on top of each other.  That makes my life as a reviewer easier :) That's said, I don't want to commit to a big solution, where the author doesn't want to explain why there are two parts of the solution instead of one.

I want you to tell me why the code that's in `VisitSymbolCast` does what it does. And the same about `updateExistingConstraints`.  Also I want to hear a solid reason why it's split this way and why we need both of them.

You should understand that I'm not peaking on you personally.  The review process takes a lot of my time too.  I want to make it easier for both of us.  When the reviewer understand what you are going for, it is much easier for them to help you in refining your solution.  This patch is very big, but the summary doesn't cover the main part: the approach.  And you leave me here dragging it out of you.


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

https://reviews.llvm.org/D103096