NoQ added inline comments.
================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h:753 - SVal V = computeObjectUnderConstruction(E, State, LCtx, CC, CallOpts, Idx); + SVal V = computeObjectUnderConstruction(E, State, currBldrCtx, LCtx, CC, + CallOpts, Idx); ---------------- tomasz-kaminski-sonarsource wrote: > tomasz-kaminski-sonarsource wrote: > > NoQ wrote: > > > You probably want an updated builder context here as well. This function > > > should be a simple wrapper, it should be completely interchangeable with > > > calling both functions directly. > > Could you please elaborate more? I would see a reason to create a context > > here if I would expect that `currBlrdCtx` refers to a different `Block` > > that we want to perform construction in. And there is no indication on > > another `Block` being inplay here, and I would construct `NodeBlockCtx` > > with same block as `currBldrCtx`. > > In other words, I expect this function to be `handeConstructionContext` in > > current `Block`. > Or to say it differently, I expect `BldCtx` not being `currBldrCtx` to be an > unusual situation, that is limited to the construction of return value. Thus > having it in `convenience` would only make it more currbesome. No-no, I mean, it's not any concrete bug that I see, it's maintaining API contracts to make the code easy to understand and make it harder to make mistakes in the future. The function `handleConstructionContext()` , by design, is supposed to be equivalent to calling `computeObjectUnderConstruction()` and `updateObjectsUnderConstruction()`. Whenever someone has to perform both steps, they can combine them together with this convenience wrapper. After your patch, they won't be able to combine the two calls in a situation when they need to pass a non-current builder context to `computeObjectUnderConstruction()`. I hope they'll be able to figure out that they need to add another parameter but I'm worried that they may think "oh this big function doesn't need that parameter, let's drop it". Because the bug you've found is quite subtle, it's easy to miss why we even need to pass that parameter. So I suggest to add the new parameter to `handleConstructionContext()` as well, and pass it manually on all call sites. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132030/new/ https://reviews.llvm.org/D132030 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
