[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Nathan Ridge via Phabricator via cfe-commits]

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