[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.

Another round of review



================
Comment at: clangd/ClangdUnit.cpp:113
 
+  CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+      : IRM(IRM) {}
----------------
ilya-biryukov wrote:
> Let's create a new empty map inside this class and have a `takeIncludeReferences` method (similar to `TopLevelDeclIDs` and `takeTopLevelDeclIDs`)
This comment is not addressed yet, but marked as done.


================
Comment at: clangd/ClangdUnit.cpp:147
+  IncludeReferenceMap &IRM;
+  std::vector<std::pair<SourceRange, std::string>> TempPreambleIncludeLocations;
 };
----------------
Nebiroth wrote:
> 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.
You copy the map for preamble and then append to it in `CppFilePreambleCallbacks`? That also LG, we should not have many references there anyway.


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


================
Comment at: clangd/ClangdUnit.cpp:141
+  std::unique_ptr<PPCallbacks> createPPCallbacks() {
+    return llvm::make_unique<IncludeRefsCollector>(*SourceMgr, IRM);
+  }
----------------
Maybe add `assert(SourceMgr && "SourceMgr must be set at this point")` here?


================
Comment at: clangd/ClangdUnit.cpp:281
+    std::shared_ptr<PCHContainerOperations> PCHs,
+    IntrusiveRefCntPtr<vfs::FileSystem> VFS, IncludeReferenceMap IRM) {
 
----------------
Don't we already store the map we need in `PreambleData`? Why do we need an extra `IRM` parameter?


================
Comment at: clangd/ClangdUnit.h:276
+std::vector<Location>
+findDefinitions(const Context &Ctx, ParsedAST &AST, Position Pos);
+
----------------
ilya-biryukov wrote:
> This function moved without need and lost a comment.
Now we have duplicated definitions here. Bad merge?


================
Comment at: clangd/CodeComplete.cpp:328
                                   unsigned NumResults) override final {
-    if (auto SS = Context.getCXXScopeSpecifier())
-      CompletedName.SSInfo = extraCompletionScope(S, **SS);
+//    if (auto SS = Context.getCXXScopeSpecifier())
+//      CompletedName.SSInfo = extraCompletionScope(S, **SS);
----------------
Accidental change?


================
Comment at: clangd/CodeComplete.cpp:807
   // Enable index-based code completion when Index is provided.
-  Result.IncludeNamespaceLevelDecls = !Index;
+  // Result.IncludeNamespaceLevelDecls = !Index;
 
----------------
Accidental change?


================
Comment at: clangd/XRefs.cpp:41
 
+  std::vector<Location> takeLocations() {
+    // Don't keep the same location multiple times.
----------------
We don't need locations anymore. Remove them.


================
Comment at: clangd/XRefs.cpp:177
 
+  std::vector<Location> IRMResult;
+  if (!AST.getIRM().empty()) {
----------------
Probably a good place for a comment. Also, maybe rename local var to something easier to understand like `IncludeDefinitions`
```
/// Process targets for paths inside #include directive.
std::vector<Location> IncludeTargets;
```


================
Comment at: clangd/XRefs.cpp:178
+  std::vector<Location> IRMResult;
+  if (!AST.getIRM().empty()) {
+    for (auto &IncludeLoc : AST.getIRM()) {
----------------
No need to special case empty maps, remove the if altogether.


================
Comment at: clangd/XRefs.cpp:183
+      unsigned CharNumber = SourceMgr.getSpellingColumnNumber(
+          DeclMacrosFinder->getSearchedLocation());
+
----------------
Replace with `DeclMacrosFinder->getSearchedLocation()` `SourceLocationBeg`, it makes the code easier to read.


================
Comment at: clangd/XRefs.cpp:185
+
+      if ((unsigned)R.start.line ==
+              SourceMgr.getSpellingLineNumber(
----------------
why do we need to convert to unsigned? To slience compiler warnings?


================
Comment at: clangd/XRefs.cpp:188
+                  DeclMacrosFinder->getSearchedLocation()) &&
+          ((unsigned)R.start.character >= CharNumber &&
+           CharNumber <= (unsigned)R.end.character)) {
----------------
this should be `R.start.charater <= CharNumber && CharNumber <= R.end.character`


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639