NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:112
+Optional<SVal> ExprEngine::retrieveFromConstructionContext(
+ ProgramStateRef State, const LocationContext *LCtx,
----------------
balazske wrote:
> baloghadamsoftware wrote:
> > baloghadamsoftware wrote:
> > > baloghadamsoftware wrote:
> > > > 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.
> > > > The only thing I can (maybe) do is to retrieve the construction context
> > > > item.
> > >
> > > This does not help even for retrieving the value, because its control
> > > logic also depends on the construction context kind. If I do it, it will
> > > be code triplication instead of duplication and results in a code that is
> > > worse to understand than the current one.
> > >
> > >
> > > 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).
> >
> > Any solution I can come up with rather adds 100 to 150 lines to the code
> > instead of saving 50. For retrieving we only have to determine the
> > construction context item (the key). For storing we also have to calculate
> > the value. It sounds good to calculate these things in separate functions
> > and then have a simple retriever and store function but there are lots of
> > recursions, double strores, non-stores, retrieving in the store function
> > that make it too complicated.
> >
> > Also retrieving is more complex than just determining the item and getting
> > the value from the context: if you look at `SimpleReturnedValueKind` and
> > `CXX17ElidedCopyReturnedValueKind` you can see that we do not use the
> > construction context we got in the parameter (the construction context of
> > the return value) but the construction context of the call instead. For
> > `ElidedTemporaryObjectKind` we take the construction context before the
> > elusion.
> >
> > Future proofness: I agree, this is a problem to solve. In cases where the
> > construction context is empty maybe we can move the calculation of the
> > values to a separate function that is called by both the "handler" and the
> > "retriever".
> Do I think correctly that `retrieveFromConstructionContext` (in the previous
> version) should return the same value as second part of
> `handleConstructionContext`? It does not look obvious at first. Or do we need
> a function with that purpose? If yes it looks a difficult task because the
> similar "logic" to get the value and update the state. Probably it is enough
> to add a flag to `handleConstructionContext` to not make new state. The
> current code looks bad because calling a "handle" type of function (that was
> private before) only to compute a value.
>
> Any solution I can come up with rather adds 100 to 150 lines to the code
> instead of saving 50.
I think the following is a fairly literal implementation of my suggestion:
{F12020285}
It's 26 lines shorter than your `retrieveFromConstructionContext()` (ok, not
50, duplicating the switch was more annoying than i expected), it adds zero new
logic (the only new piece of logic is to track constructors that were modeled
correctly but their elision wasn't; previously we could fit it into control
flow), it no longer tries to reverse-engineer the original behavior by
duplicating its logic, and it's more future-proof for the reasons explained
above.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80366/new/
https://reviews.llvm.org/D80366
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
