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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:2396
+  let Args = [VariadicStringArgument<"DiagnosticIdentifiers">];
+  let Subjects = SubjectList<[Var]>;
+  let Documentation = [AnalyzerSuppressDocs];
----------------
NoQ wrote:
> [evil mode]
> What's the expected behavior when the same variable (esp. global) has multiple redeclarations and only some of them wear the attribute?
> [/evil mode]
> 
> I guess this mostly applies to globals and we don't have much checkers that would warn on globals (maybe some of those WebKit checkers by @jkorous may be affected).
You're right, there is no way to support local variables and not globals (except for more manual approach in Sema when we assign these attributes).

As of now, it will work on the declaration/definition where the bug is reported.  It is not perfect, but I won't say that it is counter-intuitive either.


================
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
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93110