[PATCH] D38639: [clangd] #include statements support for Open definition

[William Enright via Phabricator via cfe-commits]

Nebiroth marked 8 inline comments as done.
Nebiroth added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:85
+
+    if (SourceMgr.getMainFileID() == SourceMgr.getFileID(FilenameRange.getAsRange().getBegin()) && SourceMgr.isInMainFile(FilenameRange.getAsRange().getBegin())) {
+      // Only inclusion directives in the main file make sense. The user cannot
----------------
ilya-biryukov wrote:
> ilya-biryukov wrote:
> > `clang-format` please
> Do we need both checks? Doesn't `SourceMgr.isInMainFile` handles all the cases?
This check was here to prevent an issue that appeared previously. I don't think this check is needed anymore.


================
Comment at: clangd/ClangdUnit.cpp:147
+  IncludeReferenceMap &IRM;
+  std::vector<std::pair<SourceRange, std::string>> TempPreambleIncludeLocations;
 };
----------------
ilya-biryukov wrote:
> We should have `SourceMgr` at all the proper places now, let's store `IncludeReferenceMap` directly
The idea was to pass a single map around to fill with every reference instead of having separate maps being merged together.


================
Comment at: clangd/ClangdUnit.cpp:281
+                 IntrusiveRefCntPtr<vfs::FileSystem> VFS,
+                 IncludeReferenceMap IRM) {
 
----------------
ilya-biryukov wrote:
> What reference does this `IncludeReferenceMap` contain? Includes from the preamble? 
This should contain every reference available for one file.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639