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