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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:732
+    if (Adjuster)
       DiagLevel = Adjuster(DiagLevel, Info);
 
----------------
sammccall wrote:
> 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.
I also had the same hesitation, but it looked like this is only suppressing diags based on config and tidy suppression comments (ones from header are still handled at flush).

So didn't feel like a big deal, But still it can be regression in cases when these diags have some complicated fixes attached to them (we'll compute them just for dropping on the floor).

The main reasoning for dropping the early exit was because I wanted the logic to look more uniform between diagcb and adjusters (to not think about it again when the day comes and we merge the two).

So how do you feel about filling in the diagbase in any case (which has some string copies, especially around diag message), then running `Adjuster`, `DiagCB` and `isExcluded`, and after that we perform early return before adding fixes or running include fixer?


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:785
 
-  if (isExcluded(*LastDiag))
+  if (LastDiag->Severity == DiagnosticsEngine::Ignored || isExcluded(*LastDiag))
     return;
----------------
sammccall wrote:
> (Hmm, the isExcluded case seems like it belongs next to the level adjuster logic rather than at the end.)
i agree, but didn't want to raise eyebrows :P moving it next to adjuster logic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103387