[PATCH] D79232: [analyzer] Refactor range inference for symbolic expressions

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko marked 3 inline comments as done.
vsavchenko added a comment.

> Very nice, i like this architecture.

Aww, thanks 😊

> @baloghadamsoftware @steakhal @ASDenysPetrov will you be able to plug D49074 <https://reviews.llvm.org/D49074>/D50256 <https://reviews.llvm.org/D50256>/D77792 <https://reviews.llvm.org/D77792>/D77802 <https://reviews.llvm.org/D77802>/D78933 <https://reviews.llvm.org/D78933> into this so that to separate algebra from pattern-matching and ensure no performance regressions? Or is something still missing?

Yeah, I'll be happy to hear what will be good to have for all the different cases we have and might have in the future.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:271-281
+  RangeSet VisitSymIntExpr(const SymIntExpr *Sym) {
+    return VisitBinaryOperator(Sym);
+  }
+
+  RangeSet VisitIntSymExpr(const IntSymExpr *Sym) {
+    return VisitBinaryOperator(Sym);
+  }
----------------
NoQ wrote:
> Can we replace these three with a single `VisitBinarySymExpr()`? Or is there too much template duck typing involved?
Unfortunately no, we need to know more derived types in order to use `getLHS` and `getRHS` methods. And that's why `VisitBinaryOperator` function is a template.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:340-345
+    // TODO #2: We didn't go into the nested expressions before, so it
+    // might cause us spending much more time doing the inference.
+    // This can be a problem for deeply nested expressions that are
+    // involved in conditions and get tested continuously.  We definitely
+    // need to address this issue and introduce some sort of caching
+    // in here.
----------------
NoQ wrote:
> I think this is a must-have, at least in some form. We've been exploding like this before on real-world code, well, probably not with bitwise ops but i'm still worried.
It will be pretty easy to introduce a limit on how deep we go into a tree of the given symbolic expression. That can also be a solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79232