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

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/URI.cpp:200
+  return make_string_error("Couldn't convert " + AbsolutePath +
+                           " to any given scheme.");
+}
----------------
maybe also include the schemes in the error message. with `llvm::join`?


================
Comment at: clang-tools-extra/clangd/URI.h:48
 
+  /// Creates a URI for a file using the first valid scheme from \p Schemes.
+  /// \p Schemes entries must be registered. The URI is percent-encoded.
----------------
nit: `// Similar to above except this uses the first scheme in \p Schemes that works.`


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:51
+  llvm::StringSet<> ParentURIs;
+  // Use ProximityPaths converted to URIs as proximity sources.
+  llvm::StringMap<SourceParams> RequestURIs;
----------------
this comment is no longer true.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:52
+  // Use ProximityPaths converted to URIs as proximity sources.
+  llvm::StringMap<SourceParams> RequestURIs;
+  for (const auto &Path : ProximityPaths) {
----------------
These are not URIs anymore. Consider calling it `Sources`?


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:55
+    const auto PathProximityURIs =
+        generateProximityURIs(URI::create(Path, URISchemes)->toString());
+    if (PathProximityURIs.empty())
----------------
Result of `URI::create` must be explicitly checked; it would trigger assertion if it is not checked or contains error.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:58
+      continue;
+    RequestURIs[Path] = SourceParams();
+    for (const auto &ProximityURI : PathProximityURIs)
----------------
We should still add `Path` to `RequestURIs` even if there is no proximity URI.


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:65
+  SymbolRelevanceSignals PathProximitySignals;
+  // DistanceCalculator will find the shortest distance from requested URI to
+  // any URI extracted from the ProximityPaths.
----------------
nit: `s/request URI/ProximityPaths/`


================
Comment at: clang-tools-extra/clangd/index/dex/DexIndex.cpp:242
+  auto ParsedURI = URI::parse(URIPath);
+  assert(ParsedURI &&
+         "Non-empty argument of generateProximityURIs() should be a valid "
----------------
I'd suggesting avoiding assertion here. Consider `elog` the error and returning empty result.


================
Comment at: clang-tools-extra/clangd/index/dex/Iterator.cpp:395
   for (; !It.reachedEnd(); It.advance())
-    Result.emplace_back(It.peek(), It.consume());
+    Result.emplace_back({It.peek(), It.consume()});
   return Result;
----------------
Why do we need this change?


================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:278
     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))
----------------
(Have you rebased on HEAD? The code has been changed.)


================
Comment at: clang-tools-extra/unittests/clangd/DexIndexTests.cpp:623
+
+  auto I = DexIndex::build(std::move(Builder).build(), URISchemes);
+
----------------
kbobyrev wrote:
> ioeric wrote:
> > We could use the constructor that doesn't take ownership e.g. `DexIndex({Sym1, Sym2}, URISchemes)`
> That would try to convert `{Sym1, Sym2}` to `SymbolSlab` and that's not possible. IIUC your suggestion is about using the constructor with ranges:
> 
> ```
> template <typename Range>
> DexIndex(Range &&Symbols, llvm::ArrayRef<std::string> URISchemes)
> ```
> 
> I could use `llvm::make_range(begin(Symbols), end(Symbols))`, but then I'd have to put everything into `Symbols` container first and that might be the same amount of code. I'm not sure that would be better than having a `Builder`. Did I miss something?
Have you tried std::vector<Symbol>{Sym1, Sym2}? or `push_back` twice? Using `Builder` seems unnecessary but not too bad.


https://reviews.llvm.org/D51481