[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

[Sam McCall via Phabricator via cfe-commits]

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

In D72874#1915840 <https://reviews.llvm.org/D72874#1915840>, @nridge wrote:

> I've started to update the patch to be in line with the direction discussed in the issue.
>
> @sammccall, how would you like to proceed logistically:
>
> - Do you plan to land (a possibly modified version of) D75479 <https://reviews.llvm.org/D75479>?
> - Or should I combine that patch into this one?


This patch looks good, I wouldn't bother redesigning anything, we should iterate instead.

You should go ahead, and I'll merge, and then we should work towards enabling dependent code use cases etc. SG?



================
Comment at: clang-tools-extra/clangd/FindSymbols.h:25
+/// Helper function for deriving an LSP Location from a SymbolLocation.
+llvm::Expected<Location> symbolLocationToLocation(const SymbolLocation &Loc,
+                                                  llvm::StringRef HintPath);
----------------
nit: these names are vague and echo the type signature, maybe indexToLSPLocation?


================
Comment at: clang-tools-extra/clangd/FindSymbols.h:26
+llvm::Expected<Location> symbolLocationToLocation(const SymbolLocation &Loc,
+                                                  llvm::StringRef HintPath);
+
----------------
nit: HintPath should be TUPath, the decision to use some other path as a TU path can only be made in the caller (needs context).
(Same is true for symbolToLocation, I'm not sure when that became public)


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:486
+  Req.Limit = 10;
+  TopN<ScoredLocatedSymbol, ScoredSymbolGreater> Top(*Req.Limit);
+  FuzzyMatcher Filter(Req.Query);
----------------
(The fuzzy matcher and topN are still here - I think we don't need them, right? With only up-to-3 results, std::sort seems more obvious)


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:488
+  FuzzyMatcher Filter(Req.Query);
+  Index->fuzzyFind(Req, [&](const Symbol &Sym) {
+    auto MaybeDeclLoc =
----------------
maybe bail out early (on unusable/too many) instead of doing all the score computations first?
fuzzyFind(..., {
  // bail out if it's a constructor or name doesn't match
  if (Results.size() >= 3) {
    TooMany = true;
    return;
  }
  // add result
});


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72874/new/

https://reviews.llvm.org/D72874