[PATCH] D126159: [ADT] Add edit_distance_insensitive to StringRef

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/ADT/edit_distance.h:45
 /// the given sequences into the other. If zero, the sequences are identical.
-template<typename T>
-unsigned ComputeEditDistance(ArrayRef<T> FromArray, ArrayRef<T> ToArray,
-                             bool AllowReplacements = true,
-                             unsigned MaxEditDistance = 0) {
+template <typename T, typename Functor = const T &(*)(const T &)>
+unsigned ComputeEditDistance(
----------------
I think this default templtae argument is unused? (argument deduction kicks in for both uses, doesn't it?) & wrong anyway - the functor type won't be a function pointer type, it'll be the specific type of each lambda, I think?

Could you remove this default template argument?


================
Comment at: llvm/include/llvm/ADT/edit_distance.h:86-91
+            Previous + (Map(FromArray[y - 1]) == Map(ToArray[x - 1]) ? 0u : 1u),
+            std::min(Row[x - 1], Row[x]) + 1);
       }
       else {
-        if (FromArray[y-1] == ToArray[x-1]) Row[x] = Previous;
+        if (Map(FromArray[y - 1]) == Map(ToArray[x - 1]))
+          Row[x] = Previous;
----------------
is there any concern about the number of times the map operation is used? Looks like the algorithm visits elements more than once, so might be worth some caching?

Like an easy one would probably be to compute `Map(FromArray[y-1])` outside the `for x` loop at least? (but maybe other caching should be done too, I don't know - I guess toLower is cheap enough that it's not worth much more involved caching? - I guess `Map(ToArray[x])` could be computed and cached for the next round's references to `Map(ToArray[x-1])` for instance?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126159/new/

https://reviews.llvm.org/D126159