sammccall added a comment.
In https://reviews.llvm.org/D44720#1055997, @MaskRay wrote:
> Glad you took another look. I don't want to yield, let's find another
> reviewer :)
OK - the people with the most context on this particular code are ilya-biryukov
and klimek (but klimek is out this
MaskRay added a comment.
Glad you took another look. I don't want to yield, let's find another reviewer
:)
Comment at: clangd/FuzzyMatch.cpp:230
void FuzzyMatcher::buildGraph() {
+ Scores[0][0][Miss] = Scores[0][0][Match] = {0, Miss};
for (int W = 0; W < WordN; ++W) {
sammccall added a comment.
Thanks for your work on this patch!
I think there a several useful improvements here, which I'd love to see landed.
Particularly the logic around matches that end early is much better.
There are also places that change existing design decisions in ways that don't
MaskRay added inline comments.
Comment at: clangd/FuzzyMatch.cpp:209
std::copy(NewWord.begin(), NewWord.begin() + WordN, Word);
- if (PatN == 0)
-return true;
sammccall wrote:
> similarly this one.
> (ideally we wouldn't do the work above, it's just
MaskRay updated this revision to Diff 140311.
MaskRay added a comment.
Don't rename anything. I just want to have this revision reviewed as soon as
possible
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44720
Files:
clangd/FuzzyMatch.cpp
clangd/FuzzyMatch.h
Index:
MaskRay added a comment.
In `buildGraph`, the nested loop
for (int P = 0; P < PatN; ++P) {
// Scores[P + 1][P][Miss] = Scores[P + 1][P][Match] = {AwfulScore, Miss};
for (int W = P; W < WordN; ++W) {
is interpreted as: we are calculating the cell `Scores[P+1][W+1][*]`, using the
MaskRay added inline comments.
Comment at: clangd/FuzzyMatch.cpp:93
+ for (int I = PatN; I <= WordN; I++)
+Best = std::max(Best, Scores[PatN][I][Match].Score);
if (isAwful(Best))
sammccall wrote:
> MaskRay wrote:
> > sammccall wrote:
> > > MaskRay wrote:
MaskRay updated this revision to Diff 140306.
MaskRay added a comment.
nit picking
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44720
Files:
clangd/FuzzyMatch.cpp
clangd/FuzzyMatch.h
Index: clangd/FuzzyMatch.h
MaskRay added inline comments.
Comment at: clangd/FuzzyMatch.cpp:340
+ A[WordN] = Miss;
+ for (int I = WordN, P = PatN; I > 0; I--) {
+if (I == W)
sammccall wrote:
> MaskRay wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > sammccall wrote:
> > >
MaskRay added inline comments.
Comment at: clangd/FuzzyMatch.cpp:230
void FuzzyMatcher::buildGraph() {
+ Scores[0][0][Miss] = Scores[0][0][Match] = {0, Miss};
for (int W = 0; W < WordN; ++W) {
MaskRay wrote:
> sammccall wrote:
> > MaskRay wrote:
> > >
MaskRay marked 4 inline comments as done.
MaskRay added inline comments.
Comment at: clangd/FuzzyMatch.cpp:230
void FuzzyMatcher::buildGraph() {
+ Scores[0][0][Miss] = Scores[0][0][Match] = {0, Miss};
for (int W = 0; W < WordN; ++W) {
sammccall wrote:
>
MaskRay updated this revision to Diff 140297.
MaskRay added a comment.
missScore -> missPenalty
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44720
Files:
clangd/FuzzyMatch.cpp
clangd/FuzzyMatch.h
Index: clangd/FuzzyMatch.h
sammccall added inline comments.
Comment at: clangd/FuzzyMatch.cpp:96
return None;
return ScoreScale * std::min(PerfectBonus * PatN, std::max(0, Best));
}
MaskRay wrote:
> sammccall wrote:
> > MaskRay wrote:
> > > I also don't understand why it clamps
MaskRay added inline comments.
Comment at: clangd/FuzzyMatch.cpp:96
return None;
return ScoreScale * std::min(PerfectBonus * PatN, std::max(0, Best));
}
sammccall wrote:
> MaskRay wrote:
> > I also don't understand why it clamps the value to zero here.
MaskRay added inline comments.
Comment at: clangd/FuzzyMatch.cpp:93
+ for (int I = PatN; I <= WordN; I++)
+Best = std::max(Best, Scores[PatN][I][Match].Score);
if (isAwful(Best))
sammccall wrote:
> MaskRay wrote:
> > sammccall wrote:
> > > this looks
MaskRay updated this revision to Diff 140268.
MaskRay added a comment.
Add comment
// Find the optimal prefix of Word to match Pattern.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44720
Files:
clangd/FuzzyMatch.cpp
clangd/FuzzyMatch.h
Index: clangd/FuzzyMatch.h
MaskRay updated this revision to Diff 140265.
MaskRay edited the summary of this revision.
MaskRay added a comment.
Address comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44720
Files:
clangd/FuzzyMatch.cpp
clangd/FuzzyMatch.h
Index: clangd/FuzzyMatch.h
MaskRay added inline comments.
Comment at: clangd/FuzzyMatch.cpp:230
void FuzzyMatcher::buildGraph() {
+ Scores[0][0][Miss] = Scores[0][0][Match] = {0, Miss};
for (int W = 0; W < WordN; ++W) {
sammccall wrote:
> why this change?
> this has also been moved
sammccall added a comment.
BTW if you're interested in this stuff in clangd, there's some greener-field
related stuff too:
Our goal is to be able to do project-wide fuzzy-find navigation and code
completion with the global symbol index.
The index implementation in upstream clangd is naive at
sammccall added a comment.
Sorry for the delay, it took me a while to understand exactly what everything
is doing.
If I understand right, there's actually no functional change (to match logic or
scoring) being proposed here.
But some nice fixes indeed!
Most of the comments are readability
MaskRay added a comment.
Friendly ping..
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44720
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
MaskRay added inline comments.
Comment at: clangd/FuzzyMatch.h:61
bool allowMatch(int P, int W) const;
- int skipPenalty(int W, Action Last) const;
- int matchBonus(int P, int W, Action Last) const;
+ int missScore(int W, Action Last) const;
+ int matchScore(int P, int W,
MaskRay added inline comments.
Comment at: clangd/FuzzyMatch.cpp:96
return None;
return ScoreScale * std::min(PerfectBonus * PatN, std::max(0, Best));
}
I also don't understand why it clamps the value to zero here. Negative values
are also meaningful
MaskRay added inline comments.
Comment at: clangd/FuzzyMatch.h:61
bool allowMatch(int P, int W) const;
- int skipPenalty(int W, Action Last) const;
- int matchBonus(int P, int W, Action Last) const;
+ int missScore(int W, Action Last) const;
+ int matchScore(int P, int W,
MaskRay added inline comments.
Comment at: clangd/FuzzyMatch.cpp:93
+ for (int I = PatN; I <= WordN; I++)
+Best = std::max(Best, Scores[PatN][I][Match].Score);
if (isAwful(Best))
sammccall wrote:
> this looks like a behavior change - why?
This is a
MaskRay updated this revision to Diff 139314.
MaskRay edited the summary of this revision.
MaskRay added a comment.
Update summary
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44720
Files:
clangd/FuzzyMatch.cpp
clangd/FuzzyMatch.h
Index: clangd/FuzzyMatch.h
MaskRay updated this revision to Diff 139313.
MaskRay added a comment.
Update summary
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44720
Files:
clangd/FuzzyMatch.cpp
clangd/FuzzyMatch.h
Index: clangd/FuzzyMatch.h
sammccall added a comment.
(Sorry if I sound gruff - I'm sure there are improvements to be had here. But
since the code is a bit dense (my fault) I have trouble inferring them from the
deltas.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44720
sammccall added a comment.
Can you elaborate on what this patch is improving, and how?
There are some stylistic changes, and also what look like subtle logic changes,
and some rearrangement of the algorithm - to what end?
Canonical way to run all tests is `ninja check-clang-tools`, the way you
MaskRay added a comment.
Brings some goodies from
https://github.com/cquery-project/cquery/blob/master/src/fuzzy_match.cc (what I
plagiarized from clangd)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44720
___
cfe-commits
MaskRay created this revision.
MaskRay added a reviewer: sammccall.
Herald added subscribers: cfe-commits, ioeric, jkorous-apple, ilya-biryukov,
klimek.
Also rename `matchBonus` and `skipPenalty` to uniform `{match,miss}Score` in
addition form.
Repository:
rCTE Clang Tools Extra
31 matches
Mail list logo
