This revision was automatically updated to reflect the committed changes.
Closed by commit rL339548: [clangd] Generate incomplete trigrams for the Dex
index (authored by omtcyfz, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D50517?vs
kbobyrev updated this revision to Diff 160302.
kbobyrev marked an inline comment as done.
kbobyrev edited the summary of this revision.
kbobyrev added a comment.
Address the post-LGTM comment.
https://reviews.llvm.org/D50517
Files:
clang-tools-extra/clangd/index/dex/Iterator.h
clang-tools-e
ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.
lg! Thanks for the changes!
Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:147
+ if (Chars.size() == 3) {
+const auto Trigram =
+Token(To
kbobyrev updated this revision to Diff 160154.
kbobyrev marked 7 inline comments as done.
kbobyrev added a comment.
Address a round of comments.
https://reviews.llvm.org/D50517
Files:
clang-tools-extra/clangd/index/dex/Iterator.h
clang-tools-extra/clangd/index/dex/Trigram.cpp
clang-tools-
ioeric added inline comments.
Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:83
+
+ bool FoundFirstHead = false;
+ // Iterate through valid seqneces of three characters Fuzzy Matcher can
ioeric wrote:
> It's probably unclear what `FoundFirstHead` an
ioeric added inline comments.
Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:31
+/// use it as the special symbol.
+const auto END_MARKER = '$';
+
nit: s/auto/char/
Maybe just use `static` instead of an anonymous namespace just for this.
==
kbobyrev updated this revision to Diff 160133.
kbobyrev marked 7 inline comments as done.
kbobyrev added a comment.
Address issues we have discussed with Eric.
https://reviews.llvm.org/D50517
Files:
clang-tools-extra/clangd/index/dex/Iterator.h
clang-tools-extra/clangd/index/dex/Trigram.cpp
ioeric added inline comments.
Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:33
+
+void insertChars(DenseSet &UniqueTrigrams, std::string Chars) {
+ const auto Trigram = Token(Token::Kind::Trigram, Chars);
This is probably neater as a lambda in `gene
kbobyrev updated this revision to Diff 160093.
kbobyrev marked 8 inline comments as done.
kbobyrev added a comment.
Address issues we discussed with Eric.
https://reviews.llvm.org/D50517
Files:
clang-tools-extra/clangd/index/dex/Iterator.h
clang-tools-extra/clangd/index/dex/Trigram.cpp
cl
ioeric added inline comments.
Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:74
+// symbol of the identifier.
+if (!FoundFirstSymbol) {
+ FoundFirstSymbol = true;
kbobyrev wrote:
> ioeric wrote:
> > Could this be pulled out of the loop? I
kbobyrev added a comment.
In https://reviews.llvm.org/D50517#1194990, @ioeric wrote:
> In https://reviews.llvm.org/D50517#1194976, @kbobyrev wrote:
>
> > As discussed offline with @ilya-biryukov, the better approach would be to
> > prefix match first symbols of each distinct identifier piece ins
kbobyrev added inline comments.
Comment at: clang-tools-extra/clangd/index/dex/Trigram.cpp:74
+// symbol of the identifier.
+if (!FoundFirstSymbol) {
+ FoundFirstSymbol = true;
ioeric wrote:
> Could this be pulled out of the loop? I think what we wan
kbobyrev updated this revision to Diff 160081.
kbobyrev marked 5 inline comments as done.
kbobyrev added a comment.
Address a round of comments.
I have added few comments to get additional feedback before further changes are
made.
https://reviews.llvm.org/D50517
Files:
clang-tools-extra/cla
ioeric added a comment.
In https://reviews.llvm.org/D50517#1194976, @kbobyrev wrote:
> As discussed offline with @ilya-biryukov, the better approach would be to
> prefix match first symbols of each distinct identifier piece instead of
> prefix matching (just looking at the first letter of the i
ioeric added a comment.
In https://reviews.llvm.org/D50517#1194976, @kbobyrev wrote:
> As discussed offline with @ilya-biryukov, the better approach would be to
> prefix match first symbols of each distinct identifier piece instead of
> prefix matching (just looking at the first letter of the i
kbobyrev updated this revision to Diff 160074.
kbobyrev added a comment.
@ilya-biryukov I have changed the approach to the one we discussed before.
https://reviews.llvm.org/D50517
Files:
clang-tools-extra/clangd/index/dex/Trigram.cpp
clang-tools-extra/clangd/index/dex/Trigram.h
clang-tool
ioeric added a comment.
Thanks for the patch!
In https://reviews.llvm.org/D50517#1194955, @kbobyrev wrote:
> Complete the tests, finish the implementation.
>
> One thought about prefix match suggestion: we should either make it more
> explicit for the index (e.g. introduce `prefixMatch` and dis
kbobyrev planned changes to this revision.
kbobyrev added a comment.
As discussed offline with @ilya-biryukov, the better approach would be to
prefix match first symbols of each distinct identifier piece instead of prefix
matching (just looking at the first letter of the identifier) the whole
i
kbobyrev updated this revision to Diff 160071.
kbobyrev added a comment.
Complete the tests, finish the implementation.
One thought about prefix match suggestion: we should either make it more
explicit for the index (e.g. introduce `prefixMatch` and dispatch `fuzzyMatch`
to prefix matching in c
kbobyrev planned changes to this revision.
kbobyrev added a comment.
This patch is in preview mode and can be useful for the discussion. It's not
functional yet, but this will be changed in the future.
The upcoming changes would allow handling short queries introduced in
https://reviews.llvm.or
kbobyrev created this revision.
kbobyrev added reviewers: ioeric, ilya-biryukov.
Herald added subscribers: arphaman, jkorous, MaskRay.
https://reviews.llvm.org/D50517
Files:
clang-tools-extra/clangd/index/dex/Trigram.cpp
clang-tools-extra/clangd/index/dex/Trigram.h
clang-tools-extra/unittes
21 matches
Mail list logo
