[PATCH] D93110: [analyzer] Implement fine-grained suppressions via attributes
Valeriy Savchenko via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 29 01:33:00 PST 2021
vsavchenko added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/BugSuppression.cpp:70
+
+class CacheInitializer : public RecursiveASTVisitor<CacheInitializer> {
+public:
----------------
NoQ wrote:
> vsavchenko wrote:
> > NoQ wrote:
> > > A recursive visitor would cause you to visit nested declarations (eg., lambdas, or methods of nested classes) multiple times. Is this necessary? A plain old `ConstStmtVisitor` that's taught to visit child statements recursively could easily avoid that. That's probably cheap though.
> > I don't see how I can easily use `StmtVisitor`s without A LOT OF boilerplate code that will essentially produce a `RecursiveASTVisitor` (it is very possible that I just don't know about some trick). I guess we can add some optimization if needed, but I never had performance problems with recursive visitors.
> The typical idiom is as follows:
> ```lang=c++
> void VisitChildren(const Stmt *S) {
> // Not a visitor callback - just a helper function.
> for (const Stmt *ChildS : S->children())
> if (ChildS)
> Visit(ChildS);
> }
>
> void VisitStmt(const Stmt *S) {
> // The default behavior that makes the visitor recursive over statements.
> VisitChildren(S);
> }
>
> void VisitAttributedStmt(const AttributedStmt *AS) {
> VisitAttributedNode(AS);
> VisitChildren(AS); // This *optionally* goes into every overridden function.
> }
> ```
>
> Also you'll probably have to manually unwrap `VisitVarDecl` into `VisitDeclStmt` with a loop over decls. But generally i think that's relatively little boilerplate for the much better control it provides.
>
> > I never had performance problems with recursive visitors
>
> Me too until i started doing `clang-tidy --enable-check-profile` :D So, like, i don't think that's going to be a real problem but kind of why not avoid it entirely.
That makes sense, but it's not a no-brainer tradeoff IMO. It simply gives me confidence that I'm not losing any portion of the AST out of my sight.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93110/new/
https://reviews.llvm.org/D93110
More information about the cfe-commits
mailing list