[PATCH] D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.

[Dmitri Gribenko via Phabricator via cfe-commits]

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