This revision was automatically updated to reflect the committed changes.
Closed by commit rL334810: [clangd] Boost completion score according to file
proximity. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D47935
F
ioeric added a comment.
Thanks for the review!
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47935
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ioeric updated this revision to Diff 151469.
ioeric marked 6 inline comments as done.
ioeric added a comment.
- addressed review comments and rebase.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47935
Files:
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks! Just nits
Comment at: clangd/Quality.cpp:304
OS << formatv("\tForbidden: {0}\n", S.Forbidden);
- OS << formatv("\tProximity: {0}\n", S.ProximityScore);
+ O
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/
ioeric updated this revision to Diff 151358.
ioeric marked 6 inline comments as done.
ioeric added a comment.
- addressed review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47935
Files:
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
unittests/cl
sammccall added a comment.
Thanks, just details now!
Comment at: clangd/Quality.cpp:185
+ }
+ if (F == Fs.begin() && T == Ts.begin()) // No common directory.
+return 0;
why is this a special case?
- /x/a/b vs /x/a/c is 1 up + 1 down --> 0.59
- /a/b vs /
ioeric added inline comments.
Comment at: clangd/Quality.h:71
+class SymbolRelevanceContext {
+ public:
sammccall wrote:
> This is ambiguously a couple of different (and good!) things:
> - an encapsulation of the proximitypaths state and logic
> - a grouping
ioeric updated this revision to Diff 151322.
ioeric added a comment.
- Rebase...
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47935
Files:
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
unittests/clangd/QualityTests.cpp
unittests/clangd/TestFS.cpp
uni
ioeric updated this revision to Diff 151321.
ioeric marked 2 inline comments as done.
ioeric added a comment.
- addressed review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47935
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeComplete.cpp
c
sammccall added a comment.
Thanks, this looks much clearer/more modular/more extensible to me!
A couple of notes on the abstractions before digging into details again.
Comment at: clangd/Quality.h:71
+class SymbolRelevanceContext {
+ public:
This is ambiguou
ioeric updated this revision to Diff 151172.
ioeric added a comment.
- Rebased.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47935
Files:
clangd/CodeComplete.cpp
clangd/FindSymbols.cpp
clangd/Quality.cpp
clangd/Quality.h
unittests/clangd/QualityTests.cpp
unittests
ioeric added a comment.
In https://reviews.llvm.org/D47935#1129987, @sammccall wrote:
> Sorry for the delay on this change. There's a bunch of complexity in this
> problem that I haven't seen how to slice through:
>
> 1. the signals needed seem like a weird fit for the Symbol*Signals structs
>
ioeric updated this revision to Diff 151169.
ioeric added a comment.
- Introduced a one-per-query structure for relevance signals; use
multiplication for proximity; simplify tests a bit; separate index and sema
proximity scores.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D4
sammccall added a comment.
Sorry for the delay on this change. There's a bunch of complexity in this
problem that I haven't seen how to slice through:
1. the signals needed seem like a weird fit for the Symbol*Signals structs for
some reason (maybe my misdesign)
2. the inconsistency between how
ioeric added a comment.
PTAL
Comment at: clangd/Quality.cpp:215
void SymbolRelevanceSignals::merge(const CodeCompletionResult &SemaCCResult) {
if (SemaCCResult.Availability == CXAvailability_NotAvailable ||
ioeric wrote:
> sammccall wrote:
> > proximity p
ioeric updated this revision to Diff 150924.
ioeric added a comment.
- Cleanup comment a bit.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47935
Files:
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
unittests/clangd/QualityTests.cpp
unittests/clangd/Tes
ioeric updated this revision to Diff 150923.
ioeric added a comment.
- Merge branch 'uri' into proximity
- Addressed review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47935
Files:
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
unittests/clangd
ioeric added a comment.
Here are some numbers by completing "clang::^" 40 times (with result limit 1000
instead of 100).
Timing in `CodeCompleteFlow::measureResults`
Before: Avg: 1811 us Med: 1792 us
After: Avg: 2714 us Med: 2689 us
As a reference, a full CodeCompleteFlow (with 1000 candi
ioeric added a comment.
In https://reviews.llvm.org/D47935#1126283, @sammccall wrote:
> Can you benchmark this? I'm nervous about the URI stuff in the hot path.
> Timing CodeCompleteFlow::measureResults before/after with index enabled
> seems like a reasonable test.
> (But you might want to ma
sammccall added a comment.
Oops, couple more comments.
But the big things I think are:
- what's the performance impact of doing all this work (including the URI
stuff) inside the scoring loop?
- what's the most useful formula for the proximity score
Comment at: clangd/Quality
sammccall added a comment.
Can you benchmark this? I'm nervous about the URI stuff in the hot path.
Timing CodeCompleteFlow::measureResults before/after with index enabled seems
like a reasonable test.
(But you might want to make this apply to sema first too for realistic numbers?)
===
ioeric updated this revision to Diff 150477.
ioeric added a comment.
- Rebase again...
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47935
Files:
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
unittests/clangd/QualityTests.cpp
unittests/clangd/TestFS.cpp
ioeric updated this revision to Diff 150475.
ioeric added a comment.
Rebase on https://reviews.llvm.org/D47931
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47935
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Q
ioeric created this revision.
ioeric added a reviewer: sammccall.
Herald added subscribers: cfe-commits, jkorous, MaskRay, ilya-biryukov.
Also move unittest: URI scheme to TestFS so that it can be shared by
different tests.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47935
F
25 matches
Mail list logo
