[PATCH] D60953: [clangd] Respect clang-tidy suppression comments
Nathan Ridge via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue May 14 21:25:53 PDT 2019
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
More information about the cfe-commits
mailing list