[PATCH] D48441: [clangd] Incorporate transitive #includes into code complete proximity scoring.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added a comment.

Sigh... sorry, forgot to send these comments with the last patch. Will measure performance today.



================
Comment at: clangd/FileDistance.cpp:9
+//===----------------------------------------------------------------------===//
+//
+//
----------------
ioeric wrote:
> Is this intentionally reserved?
Oops, added implementation docs


================
Comment at: clangd/FileDistance.cpp:51
+    for (StringRef Rest = Canonical; !Rest.empty();) {
+      Rest = parent_path(Rest, sys::path::Style::posix);
+      auto NextHash = hash_value(Rest);
----------------
ioeric wrote:
> nit: this probably deserve a comment or a better formatting. It looks like part of the loop incremental.
Moved out of the loop, now the loop has a counter. Hopefully less confusing?


================
Comment at: clangd/FileDistance.cpp:86
+  auto Canonical = canonicalize(Path);
+  unsigned Cost = kUnreachable;
+  SmallVector<hash_code, 16> Ancestors;
----------------
ioeric wrote:
> Should we also check the cache here?
I'm not sure why - for the full path we'll look up the cached value in the first iteration through the loop. Ancestors will be empty, so this function should be pretty cheap for this happy path - dominated by computing the hash.

or do you mean to skip canonicalization?
This doesn't matter in practice as we'll use this through URIDistance which does a pre-canonicalization cache. If we used this directly then it might be worthwhile.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48441