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