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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@vsavchenko

> It is already a pattern in other type hierarchies.

I just rarely met them. And it was hard to me reading the code searching for implementation all over the places.



================
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:
> > > ASDenysPetrov wrote:
> > > > vsavchenko wrote:
> > > > > ASDenysPetrov wrote:
> > > > > > 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.
> > > > > Re-read my comment, please.
> > > > > Oh, wait, why is it even virtual?
> > > > `ignoreCasts` is a virtual function because I haven't found any other way to implement it better.
> > > > > I don't think that it should be virtual.
> > > > Unfortunately, this is not a CRTP to avoid dynamic linking.
> > > > > Are similar functions in Expr virtual?
> > > > `SymExpr` is an abstract class. I'm not sure about similarity but `SymExpr` has such virtual methods:
> > > >   - computeComplexity
> > > >   - getType 
> > > >   - getOriginRegion
> > > > > And I think that this implementation should live in SymExpr directly.
> > > > It's impossible due to `SymExpr` implementation design. `SymExpr` knows nothing about implementation details of `SymbolCast` to invoke `ignoreCasts()`.
> > > > 
> > > a) `Expr` is also an abstract class
> > > b) I put the implementation right there in the comment above.  I don't see any reasons not to use it.
> > > c) I don't buy it about "impossible" and "implementation design" because you can always declare function in one place and define it in the other.
> > I think I achieved of what you've been concerned.
> This function should be removed then.
NP.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:3002-3004
+  std::tie(St, Sym, New) = modifySymbolAndConstraints(St, Sym, New);
+  if (!St)
+    return nullptr;
----------------
vsavchenko wrote:
> No, please, remove duplication by putting it inside of the constraint assignor.  It is designed specifically so we don't duplicate code around `assumeSymXX` functions.
+1. That's what I've recently thought about. :)


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

https://reviews.llvm.org/D103096