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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for the changes! I still find the logic quite confusing. Happy to chat offline if useful.



================
Comment at: clangd/XRefs.cpp:137
+
+IdentifiedSymbol getSymbolAtPosition(ParsedAST &AST, SourceLocation Pos) {
+  auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
----------------
this is a nice abstraction - consider hiding the DeclarationAndMacrosFinder inside it!


================
Comment at: clangd/XRefs.cpp:215
 
-  indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
-                     DeclMacrosFinder, IndexOpts);
+  // Handle goto definition for macros.
+  if (!Symbols.Macros.empty()) {
----------------
So now you're early-out if there are macros.
This means if you have

```
void log(string s) { cout << s; }
#define LOG log
LOG("hello");
```

You'll offer only line 2 as an option, and not line 1.
I'm not sure that's bad, but it's non-obvious - I think it's the thing that the comment should call out.
e.g. "If the cursor is on a macro, go to the macro's definition without trying to resolve the code it expands to"
(The current comment just echoes the code)


================
Comment at: clangd/XRefs.cpp:226
 
-  std::vector<const Decl *> Decls = DeclMacrosFinder->takeDecls();
-  std::vector<MacroDecl> MacroInfos = DeclMacrosFinder->takeMacroInfos();
+  // Handle goto definition for typical declarations.
+  //
----------------
I'm not sure this line adds anything unless you are more specific about what "typical" means, maybe just drop it.

(nit: go to, not goto)


================
Comment at: clangd/XRefs.cpp:233
+  //  - We use the location from AST and index (if available) to provide the
+  //    final results. Prefer AST over index.
+  //
----------------
because it's more up-to-date?


================
Comment at: clangd/XRefs.cpp:235
+  //
+  //  - For each symbol, we will return a location of the cannonical declaration
+  //    (e.g. function declaration in header), and a location of definition if
----------------
nit: canonical


================
Comment at: clangd/XRefs.cpp:237
+  //    (e.g. function declaration in header), and a location of definition if
+  //    they are available, definition comes first.
+  struct CandidateLocation {
----------------
first why?


================
Comment at: clangd/XRefs.cpp:246
+
+  for (auto D : Symbols.Decls) {
+    llvm::SmallString<128> USR;
----------------
nit: the body of this loop handles building the query and getting the AST locations, but they're interleaved in a confusing way - please separate them, or even make these two separate loops.

In the latter case you could put QueryRequest inside the if(Index) block, which would be easier to follow.


================
Comment at: clangd/XRefs.cpp:248
+    llvm::SmallString<128> USR;
+    if (index::generateUSRForDecl(D, USR))
+      continue;
----------------
why are we skipping the AST location if we can't generate a USR?


================
Comment at: clangd/XRefs.cpp:256
+    CandidateLocation &Candidate = ResultCandidates[ID];
+    bool IsDef = GetDefinition(D) == D;
+    // Populate one of the slots with location for the AST.
----------------
why do you not use GetDefinition(D) as the definition, in the case where it's not equal to D?


================
Comment at: clangd/XRefs.cpp:265
+  if (Index) {
+    log("query index...");
+    std::string HintPath;
----------------
remove this log message or make it more useful :-)


================
Comment at: clangd/XRefs.cpp:277
+                    // it.
+                    auto ToLSPLocation = [&HintPath](
+                        const SymbolLocation &Loc) -> llvm::Optional<Location> {
----------------
(The double-nested lambda is somewhat hard to read, can this just be a top-level function? That's what's needed to share it, right?)


================
Comment at: clangd/XRefs.cpp:516
 
   SourceLocation SourceLocationBeg = getBeginningOfIdentifier(AST, Pos, FE);
   auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
----------------
can you also use `getSymbolAtPosition` here?


================
Comment at: unittests/clangd/XRefsTests.cpp:42
 // FIXME: this is duplicated with FileIndexTests. Share it.
-ParsedAST build(StringRef Code) {
-  auto CI = createInvocationFromCommandLine(
-      {"clang", "-xc++", testPath("Foo.cpp").c_str()});
-  auto Buf = MemoryBuffer::getMemBuffer(Code);
+ParsedAST build(StringRef MainCode, StringRef HeaderCode = "",
+                StringRef BaseName = "Main",
----------------
this is too complicated, please find a way to make it simpler.
Take a look in TestFS.h for the filesystem stuff.


================
Comment at: unittests/clangd/XRefsTests.cpp:43
+ParsedAST build(StringRef MainCode, StringRef HeaderCode = "",
+                StringRef BaseName = "Main",
+                llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem>
----------------
why allow BaseName to be chosen?


================
Comment at: unittests/clangd/XRefsTests.cpp:44
+                StringRef BaseName = "Main",
+                llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem>
+                    InMemoryFileSystem = nullptr) {
----------------
why do you need to pass in the FS? The FS shouldn't need to be the same between the index and AST.


================
Comment at: unittests/clangd/XRefsTests.cpp:52
+  std::vector<const char *> Cmd = {"clang", "-xc++", MainPath.c_str()};
+  if (!HeaderCode.empty()) {
+    InMemoryFileSystem->addFile(HeaderPath, 0,
----------------
why do this conditionally?


================
Comment at: unittests/clangd/XRefsTests.cpp:72
 
+std::unique_ptr<SymbolIndex> buildIndexFromAST(ParsedAST *AST) {
+  SymbolCollector::Options CollectorOpts;
----------------
can we reuse FileIndex instead of reimplementing it?


================
Comment at: unittests/clangd/XRefsTests.cpp:162
+  // Build index.
+  auto IndexAST = build(SymbolCpp.code(), SymbolHeader.code(), "symbol",
+                        InMemoryFileSystem);
----------------
combine these into `buildIndex`? Having multiple ASTs floating around in the test seems confusing...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717