[PATCH] D45717: [clangd] Using index for GoToDefinition.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/XRefs.cpp:209
 
+  llvm::DenseSet<SymbolID> QuerySyms;
+  llvm::DenseMap<SymbolID, Location> ResultCandidates;
----------------
the approach could some documentation - I find it hard to follow the code.

This function is probably too big by now and should be split up - but without understanding the goal I can't suggest how.


================
Comment at: clangd/XRefs.cpp:216
+      log("Could not creat location for Loc");
+      continue;
+    }
----------------
this seems suspicious to me - if we can't find an AST based location for the symbol, why would that mean giving up without consulting the index?


================
Comment at: clangd/XRefs.cpp:224
+    bool IsDef = GetDefinition(D) == D;
+    // Query the index if there is no definition point in the local AST.
+    if (!IsDef)
----------------
isn't the condition here something else - that there's at least one declaration that isn't a definition?


================
Comment at: clangd/XRefs.cpp:239
+          assert(it != ResultCandidates.end());
+          if (Sym.Definition) {
+            auto Uri = URI::parse(Sym.Definition.FileURI);
----------------
why are we ignoring the index if it's not a definition (and therefore preferring the AST one?)


================
Comment at: clangd/XRefs.cpp:261
+
+  for (auto It : ResultCandidates)
+    Result.push_back(It.second);
----------------
Here we're returning exactly one candidate per symbol we identified as a candidate. There are other choices - why this one?


================
Comment at: clangd/XRefs.h:26
+std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
+                                      const SymbolIndex *const Index = nullptr);
 
----------------
nit: drop the second const here? we don't normally bother to mark by-value parameters as const (particularly not in declarations)


================
Comment at: unittests/clangd/XRefsTests.cpp:109
 
+TEST(GoToDefinition, WithIndex) {
+  MockFSProvider FS;
----------------
We have a findDefinitionsHelper to avoid all this boilerplate.
Can you extend/overload it it to work with index?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717