[clang] [Analyzer][NFC] Remove redundant function call (PR #75076)

via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 11 10:02:01 PST 2023


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


More information about the cfe-commits mailing list