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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
