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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.cpp:368
   std::vector<Location> Result;
-  Resources->getAST().get()->runUnderLock([Pos, &Result, this](ParsedAST *AST) {
-    if (!AST)
-      return;
-    Result = clangd::findDefinitions(*AST, Pos, Logger);
-  });
+  std::unordered_map<Range, Path, RangeHash> IncludeLocationMap;
+  std::vector<std::pair<SourceRange, std::string>> DataVector;
----------------
This map and two vectors come up quite often in your commit.
Is this a storage for saved includes? Could we define a class that stores this data?

Something like
```
class IncludeReferenceMap {
  llvm::Optional<PathRef> findIncludeTargetAtLoc(Location Loc);

private:
// maps, vectors, whatever we need go here ....
}; 
```

We'd build it using the callbacks, then store it in `PreambleData` (for PCH include) and `ParsedAST` (for non-PCH includes). And later query both maps to get the result.


================
Comment at: clangd/ClangdUnit.cpp:33
 #include <chrono>
+#include <iostream>
 
----------------
We probably don't need it this include


================
Comment at: clangd/ClangdUnit.cpp:38
 
+class DelegatingPPCallbacks : public PPCallbacks {
+
----------------
What's the purpose of this class?


================
Comment at: clangd/ClangdUnit.cpp:131
+
+  unsigned NumSlocs = SM.local_sloc_entry_size();
+  SmallVector<SourceLocation, 10> InclusionStack;
----------------
Please also use `PPCallbacks` interface instead of the `SourceManager` (should be possible to hook it up somewhere in `ParsedAST::Build`).


https://reviews.llvm.org/D38639