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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks, this looks good. I think we should avoid the subclassing, but any of {generalize in the next patch, different approach for the next patch, subclass for now and clean up later} seems fine.



================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:243
+// future).
+class ClangdDiagnosticConsumer : public StoreDiags {
+public:
----------------
nridge wrote:
> 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?
Sorry about not understanding the interaction here, I wasn't aware of the warnings-to-errors work.
Yes, I'd prefer to avoid the subclassing for that too.

A couple of options spring to mind here:
 - generalize from `suppressDiagnostics(callback -> bool)` to `adjustLevel(callback -> DiagnosticLevel)` or similar, where `Ignored` is the current suppression behavior.
 - apply warnings-to-errors at the end, at the clangd::Diag level. This captures that a diag came from clang-tidy, and the check name, so seems feasible.

(sorry about the churn here, I wasn't aware of the followup patch)


================
Comment at: clang-tools-extra/clangd/Diagnostics.h:115
 
+/// Check if a diagnostic is inside the main file.
+bool isInsideMainFile(const clang::Diagnostic &D);
----------------
nit: this really doesn't seem worth adding to a public interface, is `D.hasSourceManager() && D.getSourceManager().isWrittenInMainFile(D.getLocation())` really too verbose?

(I don't think the helper function in Diagnostics.cpp is justified either, but unrelated to this patch)


================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:771
+
+TEST(ClangTidy, SuppressionComment) {
+  Annotations Main(R"cpp(
----------------
nit: can we group this with the other ClangTidy test?
`TEST(DiagnosticTest, ClangTidySuppressionComment)` and move it up in the file?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953