[PATCH] D132030: [analyzer] Pass correct bldrCtx to computeObjectUnderConstruction

[Tomasz Kamiński via Phabricator via cfe-commits]

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