[PATCH] D80366: [Analyzer] Add `getReturnValueUnderConstruction()` to `CallEvent`

[Artem Dergachev via Phabricator via cfe-commits]

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