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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/include/clang/Analysis/PathDiagnosticConsumers.h:36
                 const Preprocessor &PP,                                        \
                 const cross_tu::CrossTranslationUnitContext &CTU);
+#include "clang/Analysis/PathDiagnosticConsumers.def"
----------------
gribozavr wrote:
> 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.
Will do!


================
Comment at: clang/lib/Analysis/TextDiagnostics.cpp:23
+namespace {
+class TextDiagnostics : public PathDiagnosticConsumer {
+  DiagnosticsEngine &Diag;
----------------
gribozavr wrote:
> "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?
Renamed all the stuff.

> I'm also starting to wonder whether this diagnostics infrastructure should be in its own library, not in libAnalysis -- what do you think?

Maybe! `libAnalysis` is currently a mess; it's basically a collection of all static-analyzer-ish things that are also used outside of the static analyzer. That's indeed not a good definition for a library. It incorporates a number of completely unrelated things.


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

https://reviews.llvm.org/D67422