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