[PATCH] D47935: [clangd] Boost completion score according to file proximity.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks, just details now!



================
Comment at: clangd/Quality.cpp:185
+  }
+  if (F == Fs.begin() && T == Ts.begin()) // No common directory.
+    return 0;
----------------
why is this a special case?
 - /x/a/b vs /x/a/c is 1 up + 1 down --> 0.59
 - /a/b vs /a/c is 1 up + 1 down --> 0.59
 - /b vs /c is unrelated --> 0

I don't doubt the assertion that these are unrelated paths, but I'm not sure fixing just this case is an improvement overall. (In a perfect world, we'd define the algorithm so that this case yields 0 without a discontinuity)


================
Comment at: clangd/Quality.cpp:216
+                              const FileProximityMatcher &M) {
+  OS << formatv("=== File proximity matcher ===\n");
+  OS << formatv(
----------------
For composability, you could consider styling more tersely e.g. as ProximityPaths{/path/to/file}, and in the RelevanceSignals operator<< including it like other fields, yielding:
```
== Symbol relevance: 0.8 ==
 Name match: 0.7
 File proximity matcher: ProximityPaths{/path/to/file}
 ...
```


================
Comment at: clangd/Quality.h:78
+
+    /// Calculates the best proximity score from proximity paths to the symbol's
+    /// URI. When a path cannot be encoded into the same scheme as \p SymbolURI,
----------------
Should mention the semantics of the score, maybe via the other extreme: when the SymbolURI exactly matches a proximity path, score is 1.


================
Comment at: clangd/Quality.h:98
+  /// closest.
+  float IndexProximityScore = 0;
+  llvm::StringRef IndexSymbolURI;
----------------
This is redundant with (IndexSymbolURI, FileProximityMatch) I think, and will only be correctly set if FileProximityMatch is set before calling merge(Symbol).

Can we defer calculation until evaluate()?
(If you want to dump this intermediate score, you can recompute it in operator<<, I'm not sure it's necessary).


================
Comment at: unittests/clangd/TestFS.cpp:66
 
+// Assume all files in the schema have a "test-root/" root directory, and the
+// schema path is the relative path to the root directory.
----------------
These helpers would be more coherent if this used the same test root as above - any reason we can't do that?

Then this comment could just be "unittest: is a scheme that refers to files relative to testRoot()"


================
Comment at: unittests/clangd/TestFS.cpp:107
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the plugin.
----------------
This is really surprising to me - is this the common pattern for registries?
(i.e. we don't have something more declarative like bazel's `cc_library.alwayslink`)?

If so, can we move the declaration to TestFS.h and give a usage example, so the consuming libraries don't have to repeat the decl?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47935