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