[clang] [clang][dataflow] Refactor `PropagateResultObject()` with a switch statement. (PR #88865)

via cfe-commits cfe-commits at lists.llvm.org
Mon Apr 22 05:09:56 PDT 2024


martinboehme wrote:

First of all, a followup: I should of course have noticed the failling CI tests, but a contributing factor was that I was locally running tests in `Release` mode, i.e. with `assert()` compiled out.

I've now looked at why tests fail, and it is because when using `E->getStmtClass()`, we need to enumerate all possible sub-classes that we're interested in, whereas when using `isa` / `dyn_cast`, it's enough to switch on the base class.

In practical terms, this means that to get complete coverage for what used to be covered by `isa<CXXConstructExpr>(E)`, we need to check not only for `CXXConstructExprClass` but also `CXXTemporaryObjectExprClass`. To get coverage for what used to be covered by `isa<CallExpr>(E)`, we need to check not only for `CallExprClass` but also for `CXXMemberCallExprClass` and `CXXOperatorCallExprClass`.

In my mind, this shifts the tradeoff: Having to enumerate all possible sub-classes adds verbosity and carries the risk of forgetting a subclass, particularly if subclasses are added in the future.

I would therefore propose to stay with the existing implementation as I'm no longer convinced that the switch statement is the better option.

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


More information about the cfe-commits mailing list