[PATCH] D93110: [analyzer] Implement fine-grained suppressions via attributes

[Valeriy Savchenko via Phabricator via cfe-commits]

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