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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h:30-31
+public:
+  /// Return true if the given bug report was explicitly suppressed by the user.
+  bool isSuppressed(const BugReport &);
+  /// Return true if the bug reported at the given location was explicitly
----------------
vsavchenko wrote:
> NoQ wrote:
> > So, like, i wish this could be replaced with `bool isSuppressed(const PathDiagnostic &);` so that to untie this from Static Analyzer-specific `BugReport` object.
> > 
> > There's no straightforward way to jump from `BugReport` to its specific `PathDiagnostic`, especially given that the same bug report may produce different `PathDiagnostic`s for different consumers. But by the time suppressions kick in we can be certain that `PathDiagnostic` objects are fully constructed.
> > 
> > It's an interesting question whether different `PathDiagnostic`s for the same bug report may interact with suppressions differently. For now this can't happen because all of them will have the same location and the same uniqueing location. We should obviously avoid such differences, probably protect ourselves against them with an assertion.
> In order to move fully from `BugReport` to `PathDiagnostic`, we can pre-map the whole TU with suppressed "areas" (in the same way we do it now for declarations with issues).  If we do that, we need to decide where and when such initialization should take place.
We still have access to decl-with-issue and uniqueing-decl in `PathDiagnostic` if that's what seems problematic to you (?)


================
Comment at: clang/lib/StaticAnalyzer/Core/BugSuppression.cpp:70
+
+class CacheInitializer : public RecursiveASTVisitor<CacheInitializer> {
+public:
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93110/new/

https://reviews.llvm.org/D93110