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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the useful comments! I refined the patch, and it becomes a bit larger (including some moving stuff).



================
Comment at: clangd/XRefs.cpp:137
+
+IdentifiedSymbol getSymbolAtPosition(ParsedAST &AST, SourceLocation Pos) {
+  auto DeclMacrosFinder = std::make_shared<DeclarationAndMacrosFinder>(
----------------
sammccall wrote:
> this is a nice abstraction - consider hiding the DeclarationAndMacrosFinder inside it!
Inlining DeclarationAndMacrosFinder implementation inside this function would hurt the code readability, I'd leave it as it is. Now all client sides are using this function instead of  `DeclarationAndMacrosFinder`.


================
Comment at: clangd/XRefs.cpp:215
 
-  indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
-                     DeclMacrosFinder, IndexOpts);
+  // Handle goto definition for macros.
+  if (!Symbols.Macros.empty()) {
----------------
sammccall wrote:
> 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)
This is a non-functional change.

For the above example, only line 2 will be offered. This is expected behavior IMO -- when we go to definition on a macro, users would expect the macro definition.


================
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 {
----------------
sammccall wrote:
> first why?
because this is `go to definition`, so it is sensible to return the `definition` as the first result, right?


================
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.
----------------
sammccall wrote:
> why do you not use GetDefinition(D) as the definition, in the case where it's not equal to D?
Done. Added a comment.


================
Comment at: clangd/XRefs.cpp:277
+                    // it.
+                    auto ToLSPLocation = [&HintPath](
+                        const SymbolLocation &Loc) -> llvm::Optional<Location> {
----------------
sammccall wrote:
> (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?)
Moved it to `XRef.h`, and also replace the one in `findsymbols`.


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


================
Comment at: unittests/clangd/XRefsTests.cpp:43
+ParsedAST build(StringRef MainCode, StringRef HeaderCode = "",
+                StringRef BaseName = "Main",
+                llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem>
----------------
sammccall wrote:
> why allow BaseName to be chosen?
The same reason (allowed us to use the same FS between the index and AST).


================
Comment at: unittests/clangd/XRefsTests.cpp:44
+                StringRef BaseName = "Main",
+                llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem>
+                    InMemoryFileSystem = nullptr) {
----------------
sammccall wrote:
> why do you need to pass in the FS? The FS shouldn't need to be the same between the index and AST.
I intended to use one FS for index and AST, because we'd like to test the AST case which includes the index header file, but it turns out this is not needed and add complexity of the testcode, removed it.


================
Comment at: unittests/clangd/XRefsTests.cpp:52
+  std::vector<const char *> Cmd = {"clang", "-xc++", MainPath.c_str()};
+  if (!HeaderCode.empty()) {
+    InMemoryFileSystem->addFile(HeaderPath, 0,
----------------
sammccall wrote:
> why do this conditionally?
It is unnecessary if no header code is provided. And `-include` option would affect the test somehow, if we do it unconditionally, it breaks one of the existing test cases (no location was found). I haven't digged into the reason...

```cpp
int [[i]];
#define ADDRESSOF(X) &X;
int *j = ADDRESSOF(^i);
```


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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717