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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clangd/Quality.cpp:185
+  }
+  if (F == Fs.begin() && T == Ts.begin()) // No common directory.
+    return 0;
----------------
sammccall wrote:
> 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)
The intuition is that when we hit the root, it's very likely that we are switching projects. But we could leave this out of the patch and evaluate whether this is an improvement later.


================
Comment at: clangd/Quality.h:98
+  /// closest.
+  float IndexProximityScore = 0;
+  llvm::StringRef IndexSymbolURI;
----------------
sammccall wrote:
> 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).
Done.

> (If you want to dump this intermediate score, you can recompute it in operator<<, I'm not sure it's necessary).
I think the proximity score would be useful for debugging, no?


================
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.
----------------
sammccall wrote:
> 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()"
Good idea.


================
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.
----------------
sammccall wrote:
> 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?
yeah... this pattern is also used in `tooling::CompilationDatabase` (e.g. https://github.com/llvm-mirror/clang/blob/master/lib/Tooling/CompilationDatabase.cpp#L398), and I'm not aware of a good way to deal without `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?
Done.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47935