tomasz-kaminski-sonarsource marked an inline comment as done. tomasz-kaminski-sonarsource 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); ---------------- NoQ wrote: > 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. Change applied. 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
