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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits