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

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

baloghadamsoftware marked 2 inline comments as done.
baloghadamsoftware added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:575
+  std::tie(State, RetVal) =
+    static_cast<ExprEngine*>(&Engine)->handleConstructionContext(getOriginExpr(),
+                                                         getState(),
----------------
This is extremely ugly and was one of the reasons I originally did not reuse `handleConstructionContext()`. What should I do here? Create a pure virtual `handleConstructionContext()` into `SubEngine`? Or somehow make `handleConstructionContext` static? Is there maybe a more proper way to access `ExprEngine` from `CallEvent`?


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:112
 
+Optional<SVal> ExprEngine::retrieveFromConstructionContext(
+    ProgramStateRef State, const LocationContext *LCtx,
----------------
NoQ wrote:
> Please instead re-use the code that computes the object under construction. That'll save you ~50 lines of code and will be more future-proof (eg., standalone temporaries without destructor technically have a construction context with 0 items so when we implement them correctly your procedure will stop working).
That was so my first thought. However, `handleConstructionContext()` is private and non-static. Now I tried to merge the two methods: if the value is already in the construction context, we return it, if not then we add it. Is this what you suggest? Or did I misunderstand you? At the very beginning I tried to simply use `handleConstructionContext()`, but it asserted because the value was already in the map.


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

https://reviews.llvm.org/D80366