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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
