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