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:
> > > 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.
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
