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

[Balogh, Ádám via Phabricator via cfe-commits]

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