[PATCH] D51481: [clangd] Implement proximity path boosting for Dex

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:59
     LookupTable[Sym->ID] = Sym;
-    SymbolQuality[Sym] = quality(*Sym);
+    SymbolAndScores[I] = {Sym, static_cast<float>(quality(*Sym))};
   }
----------------
ioeric wrote:
> ioeric wrote:
> > We should fix `quality` to return `float` for consistency. 
> nit: Instead of reconstruct the structure, I'd probably set the fields manually.
We shouldn't need the `static_cast<float>` anymore?


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:55
   // are stored in the descending order of symbol quality.
-  std::sort(begin(Symbols), end(Symbols),
-            [&](const Symbol *LHS, const Symbol *RHS) {
-              return SymbolQuality[LHS] > SymbolQuality[RHS];
-            });
+  std::sort(begin(ScoredSymbols), end(ScoredSymbols));
+
----------------
This sorts in ascending order by default?


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:118
+  SymbolRelevanceSignals PathProximitySignals;
+  for (const auto &ProximityPath : Req.ProximityPaths) {
+    const auto PathProximityTokens =
----------------
As chatted offline, we only need a single URIDistacne structure for all proximity paths. And we could also deduplicate parent directories.

`generateQueryPathProximityTokens` should return URIs instead of tokens as the token data doesn't need to be raw URIs (e.g. hash). And the function should probably live in DexIndex.cpp instead of Token.cpp.

I'd also suggest pulling this code into a helper.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:159
+
+  std::vector<std::pair<DocID, float>> IDAndScores = consume(*Root);
+
----------------
I think `using IDAndScore = decltype(IDAndScores.fron())` might help here.


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:287
     StaticIdx.reset(Placeholder = new SwapIndex(llvm::make_unique<MemIndex>()));
-    runAsync<void>([Placeholder] {
-      if (auto Idx = loadIndex(YamlSymbolFile))
+    runAsync<void>([Placeholder, Opts] {
+      if (auto Idx = loadIndex(YamlSymbolFile, Opts.URISchemes, UseDex))
----------------
This could take `Opts` by reference.


https://reviews.llvm.org/D51481