NoQ added a comment.
> What I want to avoid is having a static analyzer-specific suppression
> attribute, and a different one for clang-tidy, and a third one for the
> frontend.
Given that there are already multiple suppression mechanisms circulating around
(clang-tidy's `// NOLINT`, frontend pragmas, static analyzer's `#ifdef
__clang_analyzer__`, now also attribute), i guess a path forward would be to
eventually add support for multiple mechanisms everywhere. This may be tedious
to implement but benefits may be well worth it. Consistency across tools is
important when tools are used together; for instance, when clang-tidy
integrates static analyzer, static analyzer automatically starts supporting `//
NOLINT` through clang-tidy's diagnostic consumer magic. This would also be
amazing for discoverability: users can keep using their preferred mechanism and
it'd just work out of the box when they add a new tool to their arsenal.
================
Comment at: clang/include/clang/Basic/Attr.td:2396
+ let Args = [VariadicStringArgument<"DiagnosticIdentifiers">];
+ let Subjects = SubjectList<[Var]>;
+ let Documentation = [AnalyzerSuppressDocs];
----------------
[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).
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:592-593
+ /// User-provided in-code suppressions.
+ BugSuppression UserSuppressions;
+
----------------
Even though the attribute is specific to the static analyzer and from that
point of view it's appropriate to keep this info in the `BugReporter` object
(that will always be specific to the static analyzer), i believe we should
ultimately move this into a higher-level entity in `libAnalysis` such as
`AnalysisConsumer`. This would allow other tools such as clang-tidy to access
that information when integrated into the static analyzer (eg., a-la D95403)
and the user treats the entire conglomerate of tools as just "the static
analyzer" and expects consistency across checks. That, of course, relies on our
ability to look up attributes by source locations.
================
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
----------------
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.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugSuppression.h:38
+private:
+ // Overly pessimistic number, to be honest.
+ static constexpr unsigned EXPECTED_NUMBER_OF_SUPPRESSIONS = 8;
----------------
Depression fuel! :D
================
Comment at: clang/lib/StaticAnalyzer/Core/BugSuppression.cpp:70
+
+class CacheInitializer : public RecursiveASTVisitor<CacheInitializer> {
+public:
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93110/new/
https://reviews.llvm.org/D93110
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
