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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
