[PATCH] D44720: [clangd] Simplify fuzzy matcher (sequence alignment) by removing some condition checks.

[Sam McCall via Phabricator via cfe-commits]

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 suggested is the right thing for rapid iteration with unit tests.
Personally, I don't use a debugger.



================
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))
----------------
this looks like a behavior change - why?


================
Comment at: clangd/FuzzyMatch.cpp:241
 
-      auto MatchMissScore = PreMiss[Match].Score;
-      auto MissMissScore = PreMiss[Miss].Score;
-      if (P < PatN - 1) { // Skipping trailing characters is always free.
-        MatchMissScore -= skipPenalty(W, Match);
-        MissMissScore -= skipPenalty(W, Miss);
-      }
+      auto MatchMissScore = PreMiss[Match].Score + missScore(W, Match);
+      auto MissMissScore = PreMiss[Miss].Score + missScore(W, Miss);
----------------
adding the penalty unconditionally seems like a behavior change, why?


================
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, Action Last) const;
----------------
FWIW, I don't think this is an improvement - I think the clarity of purpose in names is more useful than having consistent signs in this case.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44720