Re: [PATCH] add 'string-distance' to calculate Levenshtein distance

[Eli Zaretskii]

> From: chen bin <address@hidden>
> Date: Tue, 17 Apr 2018 22:31:20 +1000
> Cc: address@hidden
> 
> As you suggested, I re-write the code using 'FETCH_STRING_CHAR_ADVANCE'.

Thanks.

> I also implemented the byte comparing version. It's 4 times as fast. And I do
> need use it to compare file path in my package 'counsel-etags'.

File names are just strings for this purpose, and they can potentially
include any non-zero characters.  So I don't see why they are special.

> The fille path couldn't contain any funny characters (emoji). so
> it'sperfectly fine
> to use byte comparing version.

File names can very well include emoji and other "funny" characters,
Emacs supports that on all modern systems (including even MS-Windows).

> diff --git a/etc/NEWS b/etc/NEWS
> index 5aa92e2991..3cce2c48c7 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -490,6 +490,8 @@ x-lost-selection-hooks, x-sent-selection-hooks
>  +++
>  ** New function assoc-delete-all.
>  
> +** New function string-distance to calculate Levenshtein distance between 
> two strings.

This long line should be filled using the fill-column setting we use
in NEWS.  Even better, make the header a short summary, like

  ** New function 'string-distance'

and then describe its functionality in a separate sentence that starts
immediately below that header.

> +DEFUN ("string-distance", Fstring_distance, Sstring_distance, 2, 3, 0,
> +       doc: /* Return Levenshtein distance between STRING1 and STRING2.
> +If BYTECOMPARE is nil, we compare character of strings.
> +If BYTECOMPARE is t, we compare byte of strings.

Please lose the "we" part, it's inappropriate in documentation,
because it describes what Emacs does.

> +Comparing by byte is faster and non-ascii characters has weighted distance.

I would delete this sentence, it is IMO confusing more than anything
else.  (And I still think the bytewise comparison is not needed.)

> +  bool use_bytecompare = !NILP(bytecompare);
                                ^^
Space between these 2 characters.

> +  else
> +    {
> +      int c1, c2;
> +      ptrdiff_t i1, i1_byte, i2, i2_byte;
> +      i2 = i2_byte = 0;
> +      for (x = 1; x <= len2; x++)

Please move the initialization of i2 and i2_byte into the for-loop
initializer (suing the comma operator).

> +          i1 = i1_byte = 0;
> +          for (y = 1, lastdiag = x - 1; y <= len1; y++)

Likewise here with i1 and i1_byte.

Thanks.