gribozavr added inline comments.
================
Comment at: clang/include/clang/Analysis/PathDiagnosticConsumers.def:23
+ANALYSIS_DIAGNOSTICS(PLIST_HTML, "plist-html", "Output analysis results using
HTML wrapped with Plists", createPlistHTMLDiagnosticConsumer)
+ANALYSIS_DIAGNOSTICS(SARIF, "sarif", "Output analysis results in a SARIF
file", createSarifDiagnosticConsumer)
+ANALYSIS_DIAGNOSTICS(TEXT, "text", "Text output of analysis results",
createTextPathDiagnosticConsumer)
----------------
80 columns?
================
Comment at: clang/include/clang/Analysis/PathDiagnosticConsumers.h:36
const Preprocessor &PP,
\
const cross_tu::CrossTranslationUnitContext &CTU);
+#include "clang/Analysis/PathDiagnosticConsumers.def"
----------------
Maybe not quite an appropriate comment for this patch, but I just realized that
this factory function returns void, which confused me -- where does the new
consumer go? Only after reading an implementation of one of these functions I
understood that they add the new consumer to `C`, which is a vector (whose type
is conveniently hidden by a typedef -- why?)
I'd suggest to make these factories more like factories, and make them return a
unique_ptr that the caller can put wherever they need. Of course, in a separate
patch.
I'd also suggest to remove the `PathDiagnosticConsumers` typedef altogether. It
is used just a couple of times in the code, and obfuscates code more than it
helps.
================
Comment at: clang/lib/Analysis/PlistHTMLDiagnostics.cpp:27
+ const cross_tu::CrossTranslationUnitContext &CTU) {
+ // Duplicate the DiagOpts object into both consumers.
+ createHTMLDiagnosticConsumer(PathDiagnosticConsumerOptions(DiagOpts), C,
----------------
Passing it by value would have made it less subtle, and probably as efficient.
================
Comment at: clang/lib/Analysis/TextDiagnostics.cpp:23
+namespace {
+class TextDiagnostics : public PathDiagnosticConsumer {
+ DiagnosticsEngine &Diag;
----------------
"TextDiagnosticsConsumer"
Or even better, "TextPathDiagnosticsConsumer".
Same for the file name. Same for other files that define diagnostics consumers.
Clarity is really important here. Very few places in the code will mention this
type by name, however, it is really important to distinguish this diagnostics
infrastructure from the rest of Clang's diagnostics.
I'm also starting to wonder whether this diagnostics infrastructure should be
in its own library, not in libAnalysis -- what do you think?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67422/new/
https://reviews.llvm.org/D67422
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
