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

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

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:112
 
+Optional<SVal> ExprEngine::retrieveFromConstructionContext(
+    ProgramStateRef State, const LocationContext *LCtx,
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > baloghadamsoftware wrote:
> > > 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.
> > I'd like to preserve the assertion that objects-under-construction are never filled twice; it's a very useful sanity check. What you need in your checker is a function that computes object-under-construction but doesn't put it into the objects-under-construction map. So you have to separate the computation from filling in the state.
> OK, so I (fortunately) misundertood you. Thus I should refactor this function to a calculation and a storing part?
OK, I see what you are speaking about, but I have no idea how to do it properly. The problem is that the control logic of filling in the state also depends on the kind of the construction context. For some kinds we do not fill at all. Every way I try it becomes more complex and less correct:

1) `NewAllocatedObjectKind`: we do not add this to the state, we only retrieve the original.
2) `SimpleReturnedValueKind` and `CXX17ElidedCopyReturnedValueKind`: depending on whether we are in top frame we handle this case recursively or we do not fill at all, just return the value. What is the construction context item here? Maybe the `ReturnStmt`?
3) `ElidedTemporaryObjectKind`: this is the most problematic: we first handle it recursively for the construction context after elision, then we also fill it for the elided temporary object construction context as well.

The only thing I can (maybe) do is to retrieve the construction context item. But then the switch is still duplicated for filling, because of the different control logic for different construction context kinds.


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

https://reviews.llvm.org/D80366