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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
