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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/AST.h:32
+/// Returns true iff all redecls of \p D are in the main file.
+bool allDeclsInMainFile(const Decl *D);
 
----------------
Do you expect this to be reused?
If so, unit test? Otherwise this seems small enough to move where it's used.


================
Comment at: clangd/Quality.cpp:24
+  if (SemaCCResult.Declaration)
+    AllDeclsInMainFile = allDeclsInMainFile(SemaCCResult.Declaration);
   if (SemaCCResult.Availability == CXAvailability_Deprecated)
----------------
ioeric wrote:
> Could you explain why `AllDeclsInMainFile` is necessary? I think it might still be fine to boost score for symbols with a declaration in the main file even if it has redecls in other files (e.g. function fwd in headers).
Agree. I think the better signal is (any) decl in main file.


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:965
 
+TEST(CompletionTest, BoostCurrentFileDecls) {
+  MockFSProvider FS;
----------------
This should really be unit tested in QualityTests. I think instead rather than in addition - I'd like to get away from code completion ranking tests for every quality signal, it's really indirect and hard to maintain/review.


================
Comment at: unittests/clangd/CodeCompleteTests.cpp:966
+TEST(CompletionTest, BoostCurrentFileDecls) {
+  MockFSProvider FS;
+  FS.Files[testPath("foo.h")] = R"cpp(
----------------
(in QualityTests, we should be able to continue using TestTU)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46943