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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:552
+
+  Index = StackFrame->getIndex();
+
----------------
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


================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:553
+
+  if(const auto Ctor = (*Block)[Index].getAs<CFGConstructor>()) {
+    return Ctor->getConstructionContext();
----------------
nit: space after `if`

And I would also prefer putting `const auto *Ctor`


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:247-249
+      Optional<SVal> ExistingVal = getObjectUnderConstruction(State, CE, LCtx);
+      if (ExistingVal.hasValue())
+        return std::make_pair(State, *ExistingVal);
----------------
Maybe here (and further) we can use a widespread pattern of `bool`-castable things declared in the `if` condition (like you do with `dyn_casts`):

```
if (Optional<SVal> ExistingVal = getObjectUnderConstruction(State, CE, LCtx))
  return std::make_pair(State, *ExistingVal);
```


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:345-360
+      if (const auto *CE = dyn_cast<CallExpr>(E)) {
+        Optional<SVal> ExistingVal =
+          getObjectUnderConstruction(State, {CE, Idx}, LCtx);
+        if (ExistingVal.hasValue())
+          return std::make_pair(State, *ExistingVal);
+      } else if (const auto *CCE = dyn_cast<CXXConstructExpr>(E)) {
+        Optional<SVal> ExistingVal =
----------------
It seems like a code duplication to me.

First of all we can first compose a `ConstructionContextItem` from either `CE`, `CCE`, or `ME` and then call for `getObjectUnderConstruction`.
Additionally, I think we can hide even that by introducing an overload for `getObjectUnderConstruction` that takes an expression and an index. 


================
Comment at: clang/unittests/StaticAnalyzer/TestReturnValueUnderConstruction.cpp:24
+public:
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const {
+    if (!Call.getOriginExpr())
----------------
martong wrote:
> I assume this tests this call expression: `returnC(1)` . But this is absolutely not trivial from the test code, could you please make this cleaner?
I think that this function, the meat and bones of the test, should be properly commented and state explicitly what are the assumption and what is the expected outcome.


================
Comment at: clang/unittests/StaticAnalyzer/TestReturnValueUnderConstruction.cpp:29
+    Optional<SVal> RetVal = Call.getReturnValueUnderConstruction(0);
+    assert(RetVal);
+    assert(RetVal->getAsRegion());
----------------
martong wrote:
> I think it would make the tests cleaner if we had some member variables set here and then in the test function (`addTestReturnValueUnderConstructionChecker`) we had the expectations on those variables.
> Right now, if we face an assertion it kills the whole unit test suite and it is not obvious why that happened.
Why not `gtest`s assertions? Those will also interrupt the execution, but will be much clearer in the output.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80366/new/

https://reviews.llvm.org/D80366