balazske added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:112 +Optional<SVal> ExprEngine::retrieveFromConstructionContext( + ProgramStateRef State, const LocationContext *LCtx, ---------------- 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. 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
