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

[Nathan James via Phabricator via llvm-commits]

njames93 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(
----------------
dblaikie wrote:
> Do you need the default type argument here? The default (or explicit, in the other call) value below would allow template argument deduction, right?
The default value doesn't seem to deduce the template argument, however explicitly passing an argument would deduce the type correctly.


================
Comment at: llvm/include/llvm/ADT/edit_distance.h:49
+    unsigned MaxEditDistance = 0,
+    Functor Map = +[](const T &X) -> const T & { return X; }) {
   // The algorithm implemented below is the "classic"
----------------
dblaikie wrote:
> I'm not sure this `+` is worthwhile - I'd say either make it a non-template entirely, and hardcode this parameter as a function pointer type (that'd work for the two callers here) or make the functor a template parameter and drop the `+` here (so that there's no call indirection overhead). (I guess a third option would be llvm::function_ref for functor-level generality without the template, but somewhat more runtime overhead, probably)
> 
> I don't have /super/ strong feelings either way.
You're right, the + isn't necessary


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126159