NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:552
+
+ Index = StackFrame->getIndex();
+
----------------
baloghadamsoftware wrote:
> 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?
It's us anyway. `StackFrameContext` is essentially analyzer-exclusive but even
for things like CFG and analysis-based warnings in Clang, there's nobody else
who maintains them except for the analyzer gang.
================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:553
+
+ if(const auto Ctor = (*Block)[Index].getAs<CFGConstructor>()) {
+ return Ctor->getConstructionContext();
----------------
baloghadamsoftware wrote:
> vsavchenko wrote:
> > nit: space after `if`
> >
> > And I would also prefer putting `const auto *Ctor`
> `Ctor` is not a pointer, but an `llvm::Optional`.
Ah, [[ https://reviews.llvm.org/D33672?id=171506#inline-475812 | classic ]].
================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:543
+
+ const StackFrameContext *StackFrame = Call.getCalleeStackFrame(BlockCount);
+ if (!StackFrame)
----------------
It is clear that the final return value of `getConstructionContext()` does not
depend on the current block count; in fact the information you're trying to
obtain is purely syntactic. You probably might as well pass 0 here. As i
mentioned before, the ideal solution would be to make `CallEvent` carry a
`CFGElementRef` to begin with (as a replacement for the origin expr), so that
it didn't need to be recovered like this.
We should absolutely eliminate the `BlockCount` parameter of
`getReturnValueUnderConstruction()` as well. Checker developers shouldn't
understand what it is, especially given that it doesn't matter at all which
value do you pass.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80366/new/
https://reviews.llvm.org/D80366
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
