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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

A few more nits (and some ideas for further restructuring the merge logic).
Otherwise LG, let's land this!



================
Comment at: clangd/XRefs.cpp:215
 
-  indexTopLevelDecls(AST.getASTContext(), AST.getTopLevelDecls(),
-                     DeclMacrosFinder, IndexOpts);
+  // Handle goto definition for macros.
+  if (!Symbols.Macros.empty()) {
----------------
hokein wrote:
> 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.
Ah, so these cases can't both happen (we even have an assert elsewhere).

Still, we can treat them as independent, and it seems simpler: you can remove the if statement, the comment for the if statement, and the early return.


================
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 {
----------------
hokein wrote:
> sammccall wrote:
> > first why?
> because this is `go to definition`, so it is sensible to return the `definition` as the first result, right?
Again, I don't think "definition" in LSP is being used in its technical c++ sense.
No issue with the behavior here, but I think the comment should be "we think that's what the user wants most often" or something - it's good to know the reasoning in case we want to change the behavior in the future.


================
Comment at: clangd/XRefs.cpp:277
+                    // it.
+                    auto ToLSPLocation = [&HintPath](
+                        const SymbolLocation &Loc) -> llvm::Optional<Location> {
----------------
hokein wrote:
> 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`.
This is pretty weird in terms of layering I think :-(
The function is pretty trivial, but depends on a bunch of random stuff.
Can we move it back (and live with the duplication) until we come up with a good home?


================
Comment at: clangd/XRefs.cpp:244
+  //    final results. When there are duplicate results, we prefer AST over
+  //    index because AST is more up-to-update.
+  //
----------------
up-to-update -> up-to-date


================
Comment at: clangd/XRefs.cpp:257
+  //   4. Return all populated locations for all symbols, definition first.
+  struct CandidateLocation {
+    llvm::Optional<SymbolID> ID;
----------------
OK, as discussed offline the bit that remains confusing is why this isn't a map.
The reason is that some symbols don't have USRs and therefore no symbol IDs.

In practice we don't really expect  multiple symbol results at all, so what about one of:
 - `map<Optional<SymbolID>, CandidateLocations>`
 - `map<SymbolID, CandidateLocations>` and use `SymbolID("")` as a sentinel

I slightly prefer the latter because it minimizes the invasiveness of handling this rare/unimportant case.


================
Comment at: clangd/index/FileIndex.h:26
 
+/// \brief Retrieves namespace and class level symbols in \p AST.
+SymbolSlab indexAST(ParsedAST *AST);
----------------
nit: prefer to remove `\brief` in favor of formatting the comment so the summary gets extracted.


================
Comment at: clangd/index/FileIndex.h:26
 
+/// \brief Retrieves namespace and class level symbols in \p AST.
+SymbolSlab indexAST(ParsedAST *AST);
----------------
sammccall wrote:
> nit: prefer to remove `\brief` in favor of formatting the comment so the summary gets extracted.
This isn't the main interface of the file.
Move to the bottom, and note that this is exposed to assist in unit tests?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45717