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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/Quality.h:52
   unsigned References = 0;
+  float ProximityScore = 0.0; /// Proximity score, in a [0,1] interval.
 
----------------
sammccall wrote:
> 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."
Hah, I totally missed the `SymbolRelevanceSignals`. Thanks, moved it there and added a comment per suggestions.


================
Comment at: unittests/clangd/QualityTests.cpp:121
 
+TEST(QualityTests, BoostCurrentFileDecls) {
+  TestTU Test;
----------------
sammccall wrote:
> consider merging into SymbolRelevanceSignalsExtraction test, which tests the same entrypoint.
> 
> If not, move up next to that one.
Merged them together. It is now a bit more verbose. In case you have more suggestions on how to properly test this, I'm happy to address them in a follow-up change.


================
Comment at: unittests/clangd/QualityTests.cpp:129
+  Test.Code = R"cpp(
+    #include "foo.h"
+    int ::test_func_in_header_and_cpp() {
----------------
sammccall wrote:
> this is not needed, the `#include` is implicit in TestTU
> 
> (and so you don't need to specify HeaderFilename either)
Done.

I did not expect the `TestTU` to do that, actually. Is this implicit `#include` something we want?
Maybe we should just have a default convention for naming the headers instead and the default `Code` value that only includes the header?
E.g. say that a file `test_header.h` is provided by TestTU and let the tests specify `#include "test_header.h"` if needed. WDYT?


================
Comment at: unittests/clangd/TestTU.cpp:80
       continue;
-    if (Result) {
+    if (Result && ND->getCanonicalDecl() != Result) {
       ADD_FAILURE() << "Multiple Decls named " << QName;
----------------
sammccall wrote:
> 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)
This function got reimplemented in one of the recent changes and works for header decls too now.
I actually think it's fine if it returns one of the overladed Decls in case there are multiple overloads (i.e. multiple Decls in the AST) for the **same** function. It does assert for multiple overloads of the same thing currently, which is probably what we want.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46943