[PATCH] D46943: [clangd] Boost scores for decls from current file in completion

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Nice, simple and will admit refinements later.

Just test nits and a trivial organizational thing.



================
Comment at: clangd/Quality.cpp:22
 
+namespace {
+bool hasDeclInMainFile(const Decl &D) {
----------------
nit: per coding style use static for functions
(I'm not sure it's a great rule, but since the ns only has this function...)


================
Comment at: clangd/Quality.h:52
   unsigned References = 0;
+  float ProximityScore = 0.0; /// Proximity score, in a [0,1] interval.
 
----------------
this belongs in `SymbolRelevanceSignals` rather than this struct. In principle, SymbolQualitySignals should all be stuff we can store in a global index (which is the point of the split). I should probably add a comment to that effect :-)

You could be a little more specific on the semantics, e.g. "Proximity between the best declaration and the query location. [0-1] score where 1 is closest."


================
Comment at: unittests/clangd/QualityTests.cpp:96
 
 TEST(QualityTests, SymbolRelevanceSignalsSanity) {
   SymbolRelevanceSignals Default;
----------------
please add a test for proximity here


================
Comment at: unittests/clangd/QualityTests.cpp:121
 
+TEST(QualityTests, BoostCurrentFileDecls) {
+  TestTU Test;
----------------
consider merging into SymbolRelevanceSignalsExtraction test, which tests the same entrypoint.

If not, move up next to that one.


================
Comment at: unittests/clangd/QualityTests.cpp:129
+  Test.Code = R"cpp(
+    #include "foo.h"
+    int ::test_func_in_header_and_cpp() {
----------------
this is not needed, the `#include` is implicit in TestTU

(and so you don't need to specify HeaderFilename either)


================
Comment at: unittests/clangd/QualityTests.cpp:155
+
+  /// Check the boost in proximity translates into a better score.
+  EXPECT_LE(FuncInHeader.evaluate(), FuncInCpp.evaluate());
----------------
this tests end-to-end, but the other tests verify input -> signals and signal -> score separately.

I'd prefer to keep (only) doing that, for consistency and because it's important we know/assert precisely what each half does so we can actually debug.


================
Comment at: unittests/clangd/TestTU.cpp:80
       continue;
-    if (Result) {
+    if (Result && ND->getCanonicalDecl() != Result) {
       ADD_FAILURE() << "Multiple Decls named " << QName;
----------------
well, I definitely wanted to flag this as an error (for the tests where this function was introduced).

Actually I think this is wrong for your test anyway - don't you want to test that no matter which decl the code completion result refers to, you get the same boost?

I'd suggest adding a `findDecls()` function that returns a vector<NamedDecl*>, and implementing `findDecl()` in terms of it. In the `test_func_in_header_and_cpp` test, you could loop over `findDecls()` and run the assertions each time.

(I guess findDecls() should assert that the returned vector is non-empty? Slightly weird but might catch accidentally vacuous tests)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46943