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