baloghadamsoftware marked 9 inline comments as done.
baloghadamsoftware added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:552
+
+ Index = StackFrame->getIndex();
+
----------------
vsavchenko wrote:
> Szelethus wrote:
> > baloghadamsoftware wrote:
> > > Szelethus wrote:
> > > > This mustn't be serious. `StackFrameContext::getIndex()` has **no
> > > > comments** at all! So it is an index to the element within a `CFGBlock`
> > > > to which it also stores a field for??? Ugh. Shouldn't we just have a
> > > > `getCallSiteCFGElement` or something? Especially since we have
> > > > `CFGElementRef`s now.
> > > >
> > > > Would you be so nice to hunt this down please? :)
> > > Do you mean a new `StackFrameContext::getCFGElement()` member function? I
> > > agree. I can do it, however this technology where we get the block and
> > > the index separately is used at many places in the code, then it would be
> > > appropriate to refactor all these places. Even wrose is the backward
> > > direction, where we look for the CFG element of a statement: we find the
> > > block from `CFGStmtMap` and then we search for the index **liearly**,
> > > instrad of storing it in the map as well!!!
> > Nasty. Yeah, I mean not to burden you work refactoring any more then you
> > feel like doing it, but simply adding a
> > `StackFrameContext::getCFGElement()` method and using it in this patch
> > would be nice enough, and some comments to `getIndex()`. But this obviously
> > isn't a high prio issue.
> +1
> I do believe that clearer interface functions and better segregation of
> different functionalities will make it much easier for us in the future
I fully agree, but StackFrameContext is not in the StaticAnalyzer part, but the
Analysis part. I think that should be a separate patch. Who is the code owner
for that part?
================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:553
+
+ if(const auto Ctor = (*Block)[Index].getAs<CFGConstructor>()) {
+ return Ctor->getConstructionContext();
----------------
vsavchenko wrote:
> nit: space after `if`
>
> And I would also prefer putting `const auto *Ctor`
`Ctor` is not a pointer, but an `llvm::Optional`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80366/new/
https://reviews.llvm.org/D80366
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
