[PATCH] D85753: [clangd] Discard diagnostics from another SourceManager.

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.


================
Comment at: clang-tools-extra/clangd/Diagnostics.cpp:209
   // Update diag to point at include inside main file.
   D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str();
   D.Range = std::move(R);
----------------
kadircet wrote:
> adamcz wrote:
> > kadircet wrote:
> > > can't we rather record the mainfilename once and make use of it here?
> > > i believe the rest of the logic operates on the local sourcelocations/files associated with the sourcemanager inside the diagnostic in process.
> > getMainFileRange() doesn't.
> > 
> > We can easily get main file from the original source manager or record it in BeginSourceFile. The thing is that it's not easy to find a place in the main file to put this diagnostic at. We can, of course, put it at the top of the file, but normally we try to find the #include location in the main file. That's done in the getMainFileRange() above and it mixes the location of the diagnostic (which is for the new SourceManager) and the main file buffer (which is for the OrigSrcMgr).
> > 
> > I'm a bit worried that if we put a diagnostic relating to #include at the top of the file, where another #include may be, it will become very confusing.
> > getMainFileRange() doesn't.
> 
> ah right i forgot about that bit :/
> 
> > I'm a bit worried that if we put a diagnostic relating to #include at the top of the file, where another #include may be, it will become very confusing.
> 
> I totally agree.
> 
> A crazy idea, we can register a callback to PP on `BeginSourceFile` to track file changes, than whenever we see a diagnostic from a different sourcemanager, we can map it back to last position a filechange was triggered. Ofc, this goes with the assumption that we see a filechanged event on `#include "modular.h"` line, but I guess we should at least get an includedirective callback or something.
Right, that's a good idea, thanks. Sam suggested something like that in parallel, during in-person discussion too. I'll look into that in the near future.

There's the other issue of only seeing this diagnostics when a module is built, but not when a module was loaded from cache, so we will not be consistent unless we serialize them somehow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85753