[PATCH] D103387: [clangd] Fix feature modules to drop diagnostics

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:732
+    if (Adjuster)
       DiagLevel = Adjuster(DiagLevel, Info);
 
----------------
If I'm reading this right:
 - we previously discarded the diagnostic "quickly" without creating a Diag
 - now, we construct the Diag as normal but never emit it
 - ... but we still use the old early-exit codepath for notes, if the primary diagnostic will be discarded

The new scheme seems functionally correct, but it potentially constructs more Diags. Question is, is it ever enough to care about performance?

I believe *warnings in headers* are ultimately filtered out by the adjuster and are now extra work/allocations after this patch. Mostly this only affects preamble (exceptions: things like llvm tablegen includes) which is already slow, maybe it doesn't matter.

If you want to avoid regressing it, I'd suggest keeping the early bail out here, but instead of setting LastPrimaryDiagnosticWasSuppressed, just construct an empty Diag and set its level to Ignored. Rest of the logic can stay the same I think.

Doesn't seem like much extra code, but if you don't think it's worthwhile that's fine too of course.


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:785
 
-  if (isExcluded(*LastDiag))
+  if (LastDiag->Severity == DiagnosticsEngine::Ignored || isExcluded(*LastDiag))
     return;
----------------
(Hmm, the isExcluded case seems like it belongs next to the level adjuster logic rather than at the end.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103387