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

Gábor Spaits via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 11 11:57:25 PST 2023


spaits wrote:

My rationale behind this change is that, as I mentioned this class is for a very specific purpose. It is a module of sub-system, not a generic "library" class that is intended to be used from anywhere, so this class would not really make sense to be used anywhere else.

If we take a look at the definition of the `ExpressionHandler` base class in `BugReporterVisitor.h`, then we can see that the third argument should be the node where the evaluation of the expression happens. We should not look for that node in the function. It is not the function's responsibility but the caller's responsibility.
```C++
  /// Handle the given expression from the given node.
  ///
  /// \param E The expression value which we are tracking
  /// \param Original A node "downstream" where the tracking started.
  /// \param ExprNode A node where the evaluation of \c E actually happens.
  /// \param Opts Tracking options specifying how we are tracking the value.
  virtual Tracker::Result handle(const Expr *E, const ExplodedNode *Original,
                                 const ExplodedNode *ExprNode,
                                 TrackingOptions Opts) = 0;
```
So there is already a written precondition for that parameter. I think the best way to express this precondition in C++17 is an assertion. If the user of the function call it incorrectly they should be warned as strongly as possible. With the least amount of runtime cost. Now we could save the cost for building a call stack, a comparison, a conditional jump based on the result of the previous comparison and also the deconstruction of the stack.

I also respect and accept your opinions and understand why you prefer that extra function call.

I will wait a bit with this PR. If no one else will review it in the next few weeks I will just abort it.

Thank you for your time and effort.

https://github.com/llvm/llvm-project/pull/75076


More information about the cfe-commits mailing list