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 {
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 

(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?

  rG LLVM Github Monorepo


cfe-commits mailing list

Reply via email to