nridge added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:243
+// future).
+class ClangdDiagnosticConsumer : public StoreDiags {
+public:
----------------
sammccall wrote:
> Thanks, this is much cleaner.
>
> I think we don't need the subclass at all though - just a filter function
> here, and call setFilter after setting up clang-tidy?
I rely on the derived class more in the follow-up patch D61841 where I override
`HandleDiagnostic()`. Should I still remove it from this patch?
================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:309
if (!LangOpts || !Info.hasSourceManager()) {
IgnoreDiagnostics::log(DiagLevel, Info);
----------------
sammccall wrote:
> do we need to set LastErrorWasIgnored here (in the case that it's not a note)?
>
> I guess it's pretty unlikely that e.g. a problem with the command-line will
> get a note that has a source location...
If that does happen, then it looks like storing the note in that case is a
pre-existing behaviour, which this patch just preserves.
================
Comment at: clang-tools-extra/clangd/Diagnostics.h:132
llvm::Optional<Diag> LastDiag;
+ bool LastErrorWasIgnored = false;
};
----------------
sammccall wrote:
> I think we're talking about last non-note diagnostic, right? Warnings count
> here I think.
>
> At the risk of being verbose, maybe `LastPrimaryDiagnosticWasIgnored`?
I copied the name from `ClangTidyDiagnosticConsumer::LastErrorWasIgnored`, but
sure, I can rename this as suggested.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60953/new/
https://reviews.llvm.org/D60953
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
