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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1203
+    if (!Opts.ShouldSupportSymbolicIntegerCasts)
+      return VisitSymExpr(Sym);
+
----------------
ASDenysPetrov wrote:
> vsavchenko wrote:
> > ASDenysPetrov wrote:
> > > vsavchenko wrote:
> > > > Why do you use `VisitSymExpr` here?  You want to interrupt all `Visits or... I'm not sure I fully understand.
> > > Here we want to delegate the reasoning to another handler as we don't support non-integral cast yet.
> > You are not delegating it here.  `Visit` includes a runtime dispatch that calls a correct `VisitTYPE` method.  Here you call `VisitSymExpr` directly, which is one of the `VisitTYPE` methods.  No dispatch will happen, and since we use `VisitSymExpr` as the last resort (it's the most base class, if we got there, we don't actually support the expression), you interrupt the `Visit` and go directly to "the last resort".
> > 
> > See the problem now?
> OK. I reject this idea before. If we call `Visit` inside `VisitSymbolCast`, we will go into recursive loop, because it will return us back to `VisitSymbolCast` as we have passed `Sym` as is. (This is theoretically, I didn't check in practice.) Or I'm missing smth?
> I choosed `VisitSymExpr` here because all kinds of `SymbolCast` were previously handled here. So I decided to pass all unsupproted forms of casts there.
Did I suggest to `Visit(Sym)`?  Of course it is going to end up in a loop!  
Why isn't it `Visit(Sym->getOperand())` here?  Before we started producing casts, casts were transparent.  This logic would fit perfectly with that.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1216
+      if (!T->isIntegralOrEnumerationType())
+        return VisitSymExpr(Sym);
+
----------------
vsavchenko wrote:
> Same goes here.
And here, since we couldn't really reason about it, we usually return `infer(T)`.


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

https://reviews.llvm.org/D103096