[PATCH] D35109: [Analyzer] SValBuilder Comparison Rearrangement

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

The reason why i don't want to commit the MAX/4 approach now (for `>`/`<` case) is that it has too little useful effects until the iterator checker is enabled by default. However, it is a core change that immediately affects all users with all its negative effects (such as performance and code complexity). When i say that (1) this approach has little useful effects and (2) this approach may cause performance issues, both (1) and (2) are only based on intuition (my or Devin's). If somebody investigates the impact of the MAX/4 change and shows that both concerns are in fact wrong (the approach is indeed very useful for all modeling and/or has negligible performance impact), i think it should land. Otherwise, i think it shouldn't land now, but delayed until the iterator checker is ready to be enabled by default.

For the type extension approach, somebody simply needs to do the math and prove that it works soundly in all cases. Devin has been heroically coming up with counter-examples so far, but even if he doesn't find any, it doesn't mean that it works, so ideally we shouldn't make him do this. The thing about the MAX/4 approach is that the math is fairly obvious: it is clear that overflows or truncations never occur under these constraints, therefore school algebra rules apply. If the type extension approach is proven to be sound, and covers more cases than the MAX/4 approach, i'd gladly welcome it.


https://reviews.llvm.org/D35109