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