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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, this looks nice. Main ideas here:

- it'd be nice to get rid of the subclassing in the diag consumer if we can
- i'm not sure the logic around LastErrorWasIgnored is completely accurate
- can we add a unit test for this? The existing clang-tidy diag tests are in `unittests/DiagnosticsTests.cpp`



================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:243
+// future).
+class ClangdDiagnosticConsumer : public StoreDiags {
+public:
----------------
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?


================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:338
       Check->registerMatchers(&CTFinder);
     }
   }
----------------
I believe you can just call setFilter at the end of this block


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:309
 
   if (!LangOpts || !Info.hasSourceManager()) {
     IgnoreDiagnostics::log(DiagLevel, Info);
----------------
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...


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:314
 
+  if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
+    return;
----------------
Can you add a comment here, about notes attached to suppressed errors? This code is under-commented (sorry), but I think it would aid understanding.


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:319
 
+  if (Filter && !Filter(DiagLevel, Info, InsideMainFile)) {
+    LastErrorWasIgnored = true;
----------------
If I read the code right, we're now applying the filter to a note (whose primary diagnostic was ignored), which we shouldn't. We're also updating the LastErrorWasIgnored flag, which we shouldn't for notes.

I think we might prefer to merge the new logic with the bit below the lambdas, which already looks at note/non-note status.

e.g.

```
if (!isNote(...)) {
  flushLastDiag();
  LastErrorWasIgnored = !Filter(...);
  if (LastErrorWasIgnored)
    return;
  ...
} else {
  // Handle a note...
  if (LastErrorWasIgnored)
    return;
  ...
}


================
Comment at: clang-tools-extra/clangd/Diagnostics.h:118
+      std::function<bool(DiagnosticsEngine::Level, const clang::Diagnostic &,
+                         bool IsInsideMainFile)>;
   /// If set, possibly adds fixes for diagnostics using \p Fixer.
----------------
nit: IsInsideMainFile is information already inside the diagnostic.

Diagnostics.cpp does compute this information (in an unneccesarily verbose way IMO) but I'd prefer not to expose that in this public interface - the filter function can recompute.


================
Comment at: clang-tools-extra/clangd/Diagnostics.h:122
+  /// If set, ignore diagnostics for which \p Filter returns false.
+  void filterDiagnostics(DiagFilter Filter) { this->Filter = Filter; }
 
----------------
I know I suggested this name, but just an idea...

The difficulty with `filter` is it's unclear whether `true` means "this passes the filter" or "this is filtered out". It may be clearer to invert the sense and call this `suppress`. 

Anyway, totally up to you, can definitely live with filter.


================
Comment at: clang-tools-extra/clangd/Diagnostics.h:132
   llvm::Optional<Diag> LastDiag;
+  bool LastErrorWasIgnored = false;
 };
----------------
I think we're talking about last non-note diagnostic, right? Warnings count here I think.

At the risk of being verbose, maybe `LastPrimaryDiagnosticWasIgnored`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60953