isuckatcs wrote:
If it's guaranteed that `ExprNode` is for `E`, I think it would be a good idea
to assert it in the function.
```c++
assert(ExprNode->getStmtForDiagnostics() == E);
```
However I guess this assumption is made from the only currently known callsite
of the function, which is:
```c++
Tracker::Result Tracker::track(const Expr *E, const ExplodedNode *N,
TrackingOptions Opts) {
if (!E || !N)
return {};
const Expr *Inner = peelOffOuterExpr(E, N);
const ExplodedNode *LVNode = findNodeForExpression(N, Inner); // <- Assertion
guaranteed here
if (!LVNode)
return {};
Result CombinedResult;
// Iterate through the handlers in the order according to their priorities.
for (ExpressionHandlerPtr &Handler : ExpressionHandlers) {
CombinedResult.combineWith(Handler->handle(Inner, N, LVNode, Opts));
if (CombinedResult.WasInterrupted) {
// There is no need to confuse our users here.
// We got interrupted, but our users don't need to know about it.
CombinedResult.WasInterrupted = false;
break;
}
}
return CombinedResult;
}
```
Calling the function from `Tracker::track()` indeed guarantees the assertion,
however I can't find anything that guarantees that `PRValueHandler::handle()`
cannot be called from anywhere else. In which case the assertion will fail and
we'll start seeing crashes.
While the change has no effect now, it could introduce a crash later, so I
would leave `PRValueHandler::handle()` as it is.
https://github.com/llvm/llvm-project/pull/75076
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
