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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.

Thanks for addressing the comments quickly.
I took another look and added a few more comments.
This moves in the right direction, though, this is really close to landing.



================
Comment at: clangd/ClangdServer.cpp:25
 #include <future>
+#include <unordered_map>
 
----------------
Is this include redundant now? Can we remove it?


================
Comment at: clangd/ClangdServer.cpp:430
+  Resources->getAST().get()->runUnderLock([Pos, &Result, &Ctx,
+                                           this](ParsedAST *AST) {
     if (!AST)
----------------
Capturing `this` seems redundant. Remove it?


================
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
----------------
`clang-format` please


================
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:
> `clang-format` please
Do we need both checks? Doesn't `SourceMgr.isInMainFile` handles all the cases?


================
Comment at: clangd/ClangdUnit.cpp:113
 
+  CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+      : IRM(IRM) {}
----------------
Let's create a new empty map inside this class and have a `takeIncludeReferences` method (similar to `TopLevelDeclIDs` and `takeTopLevelDeclIDs`)


================
Comment at: clangd/ClangdUnit.cpp:147
+  IncludeReferenceMap &IRM;
+  std::vector<std::pair<SourceRange, std::string>> TempPreambleIncludeLocations;
 };
----------------
We should have `SourceMgr` at all the proper places now, let's store `IncludeReferenceMap` directly


================
Comment at: clangd/ClangdUnit.cpp:281
+                 IntrusiveRefCntPtr<vfs::FileSystem> VFS,
+                 IncludeReferenceMap IRM) {
 
----------------
What reference does this `IncludeReferenceMap` contain? Includes from the preamble? 


================
Comment at: clangd/ClangdUnit.cpp:347
                              const SourceLocation &SearchedLocation,
-                             ASTContext &AST, Preprocessor &PP)
-      : SearchedLocation(SearchedLocation), AST(AST), PP(PP) {}
+                             ASTContext &AST, Preprocessor &PP, const IncludeReferenceMap &IncludeLocationMap)
+      : SearchedLocation(SearchedLocation), AST(AST), PP(PP), IncludeLocationMap(IncludeLocationMap) {}
----------------
This class handles processing AST and preprocessor, it does not need to get `IncludeLocationMap` in constructor or store it at all.
Remove `IncludeLocationMap` from this class and handle getting references from `IncludeLocationMap` outside this class instead.


================
Comment at: clangd/ClangdUnit.h:97
 
+  IncludeReferenceMap &getIRM() { return IRM; };
+
----------------
Make it all const?

`const IncludeReferenceMap &getIRM() const { return IRM; }`


================
Comment at: clangd/ClangdUnit.h:276
+std::vector<Location>
+findDefinitions(const Context &Ctx, ParsedAST &AST, Position Pos);
+
----------------
This function moved without need and lost a comment.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639