[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 inline comments.
This revision now requires changes to proceed.


================
Comment at: clangd/ClangdServer.cpp:446
   std::vector<Location> Result;
-  Resources->getAST().get()->runUnderLock([Pos, &Result, this](ParsedAST *AST) {
+  std::shared_future<std::shared_ptr<const PreambleData>> Preamble =
+      Resources->getPreamble();
----------------
We don't need this `Preamble` variable, right?


================
Comment at: clangd/ClangdUnit.cpp:73
 
-class CppFilePreambleCallbacks : public PreambleCallbacks {
+class CppFilePreambleCallbacks : public PreambleCallbacks, public PPCallbacks {
 public:
----------------
We don't need to derive this class from `PPCallbacks`, `DelegatingPPCallbacks` can call any method on it anyway as it knows the exact type. Please remove this inheritance.


================
Comment at: clangd/ClangdUnit.cpp:79
 
+  CppFilePreambleCallbacks(SourceManager *SourceMgr, IncludeReferenceMap &IRM)
+      : SourceMgr(SourceMgr), IRM(IRM) {}
----------------
Instead of passing `SourceManager` in constructor, maybe add a `BeforeExecute(CompilerInstance &CI)` method to `PreambleCallbacks` and set `SourceManager` when its called.


================
Comment at: clangd/ClangdUnit.cpp:167
+        llvm::make_unique<DelegatingPPCallbacks>(*this);
+    return DelegatedPPCallbacks;
+  }
----------------
NIT: replace with `return llvm::make_unique<DelegatingPPCallbacks>(*this);` and remove the local variable.


================
Comment at: clangd/ClangdUnit.cpp:175
+  IncludeReferenceMap &IRM;
+  std::vector<std::pair<SourceRange, std::string>> TempPreambleIncludeLocations;
 };
----------------
Adding `BeforeExecute` would allow to get rid of this vector.


================
Comment at: clangd/ClangdUnit.cpp:247
                  IntrusiveRefCntPtr<vfs::FileSystem> VFS,
-                 clangd::Logger &Logger) {
+                 clangd::Logger &Logger, IncludeReferenceMap &IRM) {
 
----------------
Let's store `IncludeReferenceMap` in `ParsedAST` instead of having it as out parameter.
It allows to properly manage the lifetime of `IncludeReferenceMap` (logically, it is exactly tied to the AST).


================
Comment at: clangd/ClangdUnit.cpp:270
+
+  CppFilePreambleCallbacks SerializedDeclsCollector(&Clang->getSourceManager(),
+                                                    IRM);
----------------
Why do we use `CppFilePreambleCallbacks` here? Factor the code that handles the references into a separate class and use it here:

```
class IncludeRefsCollector : public PPCallbacks {
public:
  IncludeRefsCollector(IncludeReferenceMap &Refs) : Refs(Refs) {}

   // implementation of PPCallbacks ...

private;
  IncludeReferenceMap &Refs;
};

/// .....
class CppFilePreambleCaallbacks {
public:
// ....

  std::unique_ptr<PPCallbacks> createPPCallbacks() { return llvm::make_unique<IncludeRefsCollector>(this->IRM); }
};

// ....
void ParsedAST::Build() {
  // ....
  // No need to create CppFilePreambleCallbacks here.
  Clang->getPreprocessor().addPPCallbacks(llvm::make_unique<IncludeRefsCollector>(IRM));
  //....
}
```


================
Comment at: clangd/ClangdUnit.cpp:309
   Preprocessor &PP;
+  std::unordered_map<Range, Path, RangeHash> IncludeLocationMap;
 
----------------
Use `IncludeReferenceMap` typedef here.


================
Comment at: clangd/ClangdUnit.cpp:315
+      Preprocessor &PP,
+      std::unordered_map<Range, Path, RangeHash> IncludeLocationMap)
+      : SearchedLocation(SearchedLocation), AST(AST), PP(PP),
----------------
- use a typedef for the map here
- accept the map by const reference to avoid copies


================
Comment at: clangd/ClangdUnit.cpp:365
     Range R = {Begin, End};
+    addLocation(URI::fromFile(
+                    SourceMgr.getFilename(SourceMgr.getSpellingLoc(LocStart))),
----------------
Do we change semantics of getting the file paths here? Maybe leave the code as is?


================
Comment at: clangd/ClangdUnit.cpp:370
+
+  void addLocation(URI Uri, Range R) {
     Location L;
----------------
This method does not seem particularly useful. One can construct location like this: `Location{Uri, R}`, the whole method can be written as `DeclarationLocations.push_back(Location{Uri, R})`.


================
Comment at: clangd/ClangdUnit.cpp:378
   void finish() override {
+
+    for (auto &IncludeLoc : IncludeLocationMap) {
----------------
NIT: empty linke at the start of the function


================
Comment at: clangd/ClangdUnit.cpp:381
+      Range R = IncludeLoc.first;
+      if (isSameLine(R.start.line)) {
+        addLocation(URI::fromFile(IncludeLoc.second), Range{Position{0,0}, Position{0,0}});
----------------
Let's check the the point is in range, not that it's not the same line. So that we don't trigger goto in strange places, like comments:

```
#include <vector> // this include is very important.  <--- we should not skip to include when editor caret is over a comment
```


================
Comment at: clangd/ClangdUnit.cpp:420
+    ParsedAST &AST, Position Pos, clangd::Logger &Logger,
+    std::unordered_map<Range, Path, RangeHash> IncludeLocationMap) {
   const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
----------------
If we store `IncludeLocationMap` in `ParsedAST` we don't need to change interface of this function.


================
Comment at: clangd/ClangdUnit.h:63
 
+using IncludeReferenceMap = std::unordered_map<Range, Path, RangeHash>;
+
----------------
We store a map, but only iterate through it in order. Let's use `vector<pair<Range, Path>>` instead.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639