[PATCH] D83178: [clangd] Send EOF before resetting diagnostics consumer

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

In D83178#2131914 <https://reviews.llvm.org/D83178#2131914>, @sammccall wrote:

> > can emit diagnostics at this callback (as mentioned in the comments).
>
> I'm a bit puzzled about this. The case that the code handles and the comment refers to is if a check installs PPCallbacks and overrides EndSourceFile. And I believe the existing code is ok for this case: we don't detach those callbacks.


right, we invoke the callback but we replace the diagnostic consumer client with `Clang->getDiagnostics().setClient(new IgnoreDiagnostics);` before invoking the callback. Hence whenever the clang-tidy check emits the diagnostics it doesn't get handled by `StoreDiags`, instead it's received by `IgnoreDiagnostics`.

> It didn't occur to me that EOF could be observed through the DiagnosticConsumer instead, and this patch does look like a reasonable fix for that. However ClangTidyDiagnosticConsumer doesn't override EndSourceFile nor even the destructor, so it doesn't seem to actually happen.
> 
> LLVM-include-order is hooking PPCallbacks::EndOfMainFile, i.e. the first case and the one I thought we were already handling, and I wouldn't expect changing the relative order of EOF and resetting the DC to matter for that.

as explained above issue isn't about diagsconsumer (or `PPCallbacks::EndOfMainFile` which is invoked by `PP.EndSourceFile()`). It is just about having the no-op diagnostics client registered when https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.cpp#L151 is hit.

> So I believe you that it fixes something but I'd feel better having some idea of how :-)
>  (We have a test with a fake clang tidy check IIRC, I wonder if we can test this there)

I was being lazy :( as that fake tidy check is not really re-usable, so we either extend the test, duplicate the logic or refactor it out. I wanted to go with the last option as (IIRC) we already have 2 tests with fake clang-tidy checkers.
Will do that though!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83178