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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
