ASDenysPetrov added inline comments.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:293-296
+ SymbolRef Sym = Operand;
+ while (isa<SymbolCast>(Sym))
+ Sym = cast<SymbolCast>(Sym)->Operand;
+ return Sym;
----------------
vsavchenko wrote:
> ASDenysPetrov wrote:
> > vsavchenko wrote:
> > >
> > Do you think the recursive call is better than the loop? But, I guess, I
> > see your point. You option could be safer if we had another implementation
> > of the virtual method. Or you think such alike cast symbol is possible in
> > the future? Well, for now `ignoreCasts` doesn't make sense to any other
> > `Expr` successors.
> Oh, wait, why is it even virtual? I don't think that it should be virtual.
> Are similar functions in `Expr` virtual?
> And I think that this implementation should live in `SymExpr` directly. Then
> it would look like:
> ```
> if (const SymbolCast *ThisAsCast = dyn_cast<SymbolCast>(this)) {
> return ThisAsCast->ignoreCasts();
> }
> return this;
> ```
Yes, `SymExpr` is an abstract class. And because of limitations and dependency
of inheritance we are not able to know the implementaion of `SymbolCast`.
Unfortunately, this is not a CRTP.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1203
+ if (!Opts.ShouldSupportSymbolicIntegerCasts)
+ return VisitSymExpr(Sym);
+
----------------
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.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1241
+ // Find the first constraint and exit the loop.
+ RSPtr = getConstraint(State, S);
+ }
----------------
vsavchenko wrote:
> ASDenysPetrov wrote:
> > vsavchenko wrote:
> > > Why do you get associated constraint directly without consulting with
> > > what `SymbolRangeInferrer` can tell you about it?
> > What do you mean? I didn't get. Could you give an example?
> `getConstraint` returns whatever constraint we have stored directly in the
> constraint map. That's the main source of information for ranges, but not
> the only one.
>
> Here is the of things that you skip, when you do `getConstraint` here:
> * we can understand that something is equality/disequality check and find
> the corresponding info in Equivalence Classes data structure
> * we can see that the expression has the form `A - B` and we can find
> constraint for `B - A`
> * we can see that the expression is comparison `A op B` and check what
> other comparison info we have on `A` and `B` (your own change)
> * we can see that the expression is of form `A op B` and check if we know
> something about `A` and `B`, and produce a reasonable constraint out of this
> information
>
> In order to use the right information, you should use `infer` that will
> actually do all other things as well. That's how `SymbolRangeInferrer` is
> designed, to be **recursive**.
>
> Speaking of **recursiveness**. All these loops and manually checking for
> types of the cast's operand is against this pattern. Recursive visitors
> should call `Visit` for children nodes (like `RecursiveASTVisitor`). In
> other words, if `f(x)` is a visit function, it should be defined like this:
> ```
> f(x) = g(f(x->operand_1), f(x->operand_2), ... , f(x->operand_N))
> ```
> or if we talk about your case specifically:
> ```
> f(x: SymbolCast) = h(f(x->Operand))
> ```
> and the `h` function should transform the range set returned by
> `f(x->Operand)` into a range set appropriate for `x`.
>
> NOTE: `h` can also intersect different ranges
Thank you for useful notes! I'll take them into account.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103096/new/
https://reviews.llvm.org/D103096
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
