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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D103096#2877818 <https://reviews.llvm.org/D103096#2877818>, @ASDenysPetrov wrote:

> Made `ignoreCast` non-virtual.
> P.S.  IMO, this change is not something that can be taken as a pattern, though.

It is already a pattern in other type hierarchies.
Virtual functions are only good, when they can have multiple implementations.  `ignoreCasts` by its name can have only one implementation and couldn't be virtual.  That's it!  It is more useable now, and less confusing for its users.  The fact that its definition lives in some other cpp file doesn't change it.



================
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;
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:3002-3004
+  std::tie(St, Sym, New) = modifySymbolAndConstraints(St, Sym, New);
+  if (!St)
+    return nullptr;
----------------
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.


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

https://reviews.llvm.org/D103096