Add grapheme_levenshtein function. #18087

youkidearitai · 2025-03-16T01:06:47Z

Measure levenshtein for grapheme cluster unit.
RFC: https://wiki.php.net/rfc/grapheme_levenshtein

youkidearitai · 2025-04-16T00:42:16Z

This RFC was accepted. I will go to implementation.

devnexen · 2025-04-16T06:01:32Z

Nice, could you possibly rebase with last master state to redo the CI ? cheers.

youkidearitai · 2025-04-16T06:23:41Z

I rebased and force pushed. I'm waiting for CI end.

devnexen · 2025-04-16T06:42:36Z

should be fine, thanks. Looking good but I ll go back at it in couple of hours.

ext/intl/grapheme/grapheme_string.c

devnexen

LGTM minus the remark

ext/intl/grapheme/grapheme_string.c

DanielEScherzer · 2025-04-16T21:49:51Z

ext/intl/grapheme/grapheme_string.c

+		if (ustring1) {
+			efree(ustring1);
+		}
+		RETURN_FALSE;


Is there a reason that false is being returned on failure, rather than throwing an exception?

For example, Userland can uses intl_get_error_message function after this function if this error. Therefore, this block is returns false.

youkidearitai · 2025-04-17T02:51:05Z

Hmm... Windows CI is failed.

ext/intl/tests/grapheme_levenshtein.phpt

ext/intl/grapheme/grapheme_string.c

nielsdos · 2025-04-17T22:04:23Z

ext/intl/grapheme/grapheme_string.c

+
+	unsigned char u_break_iterator_buffer1[U_BRK_SAFECLONE_BUFFERSIZE];
+	unsigned char u_break_iterator_buffer2[U_BRK_SAFECLONE_BUFFERSIZE];
+	bi1 = grapheme_get_break_iterator((void*)u_break_iterator_buffer1, &ustatus1);


Where are the error checks for these calls and the next ones?

The error checks seem to be still missing.

You also don't need the void* cast, you can remove that.

ext/intl/grapheme/grapheme_string.c

nielsdos · 2025-04-18T10:10:43Z

ext/intl/grapheme/grapheme_string.c

+		RETURN_THROWS();
+	}
+
+	zend_long *p1, *p2, *tmp;


Where possible, I think it's better to merge the declaration and assignment as it reduces the area/scope where something is valid/can be used.

The declarations and assignments are still not merged

ext/intl/grapheme/grapheme_string.c

nielsdos

Second review round.
As a more general comment: the code can probably be simplified even more by using goto-style error handling.

nielsdos · 2025-04-19T23:44:08Z

ext/intl/grapheme/grapheme_string.c

+			UStringSearch *srch = usearch_open(ustring1 + current1, pos1 - current1, ustring2 + current2, pos2 - current2, "", NULL, &ustatus2);
+			if (U_FAILURE(ustatus2)) {
+				intl_error_set_code(NULL, ustatus2);
+				intl_error_set_custom_msg(NULL, "Error usearch_open", 0);
+				ubrk_close(bi1);
+				ubrk_close(bi2);
+
+				efree(ustring1);
+				efree(ustring2);
+
+				efree(p1);
+				efree(p2);
+				RETURN_FALSE;
+			}
+			usrch_pos = usearch_first(srch, &ustatus2);
+			if (U_FAILURE(ustatus2)) {
+				intl_error_set_code(NULL, ustatus2);
+				intl_error_set_custom_msg(NULL, "Error usearch_first", 0);
+				ubrk_close(bi1);
+				ubrk_close(bi2);
+
+				efree(ustring1);
+				efree(ustring2);
+
+				efree(p1);
+				efree(p2);
+				RETURN_FALSE;
+			}
+			usearch_close(srch);


The entire comparison is too complex and inefficient.
You can just do something like this (I can't use a suggestion because the changes are not centralised):

diff --git a/ext/intl/grapheme/grapheme_string.c b/ext/intl/grapheme/grapheme_string.c index c4d0c3987a8..c0f21f1ad7e 100644 --- a/ext/intl/grapheme/grapheme_string.c +++ b/ext/intl/grapheme/grapheme_string.c @@ -1041,6 +1041,9 @@ PHP_FUNCTION(grapheme_levenshtein) RETURN_FALSE; } + UCollator *collator = ucol_open("", &ustatus1); + // TODO: handle error + p1 = safe_emalloc(strlen_2 + 1, sizeof(zend_long), 0); p2 = safe_emalloc(strlen_2 + 1, sizeof(zend_long), 0); @@ -1052,7 +1055,6 @@ PHP_FUNCTION(grapheme_levenshtein) int32_t current2 = 0; int32_t pos1 = 0; int32_t pos2 = 0; - int32_t usrch_pos = 0; while (true) { current1 = ubrk_current(bi1); @@ -1067,37 +1069,8 @@ PHP_FUNCTION(grapheme_levenshtein) if (pos2 == UBRK_DONE) { break; } - UStringSearch *srch = usearch_open(ustring1 + current1, pos1 - current1, ustring2 + current2, pos2 - current2, "", NULL, &ustatus2); - if (U_FAILURE(ustatus2)) { - intl_error_set_code(NULL, ustatus2); - intl_error_set_custom_msg(NULL, "Error usearch_open", 0); - ubrk_close(bi1); - ubrk_close(bi2); - - efree(ustring1); - efree(ustring2); - - efree(p1); - efree(p2); - RETURN_FALSE; - } - usrch_pos = usearch_first(srch, &ustatus2); - if (U_FAILURE(ustatus2)) { - intl_error_set_code(NULL, ustatus2); - intl_error_set_custom_msg(NULL, "Error usearch_first", 0); - ubrk_close(bi1); - ubrk_close(bi2); - - efree(ustring1); - efree(ustring2); - - efree(p1); - efree(p2); - RETURN_FALSE; - } - usearch_close(srch); - if (usrch_pos != USEARCH_DONE) { + if (ucol_strcoll(collator, ustring1 + current1, pos1 - current1, ustring2 + current2, pos2 - current2) == UCOL_EQUAL) { c0 = p1[i2]; } else { c0 = p1[i2] + cost_rep; @@ -1118,6 +1091,8 @@ PHP_FUNCTION(grapheme_levenshtein) p2 = tmp; } + ucol_close(collator); + ubrk_close(bi1); ubrk_close(bi2);

I didn't intend to use collation. Please give me some time to judge.

Thanks for suggestion. I could not find problem. Use your code.

nielsdos · 2025-04-19T23:44:29Z

ext/intl/grapheme/grapheme_string.c

+
+	unsigned char u_break_iterator_buffer1[U_BRK_SAFECLONE_BUFFERSIZE];
+	unsigned char u_break_iterator_buffer2[U_BRK_SAFECLONE_BUFFERSIZE];
+	bi1 = grapheme_get_break_iterator((void*)u_break_iterator_buffer1, &ustatus1);


The error checks seem to be still missing.

nielsdos · 2025-04-19T23:44:39Z

ext/intl/grapheme/grapheme_string.c

+		RETURN_THROWS();
+	}
+
+	zend_long *p1, *p2, *tmp;


The declarations and assignments are still not merged

nielsdos · 2025-04-19T23:45:02Z

ext/intl/grapheme/grapheme_string.c

+	int32_t ustring2_len = 0;
+
+	UErrorCode ustatus1 = U_ZERO_ERROR;
+	UErrorCode ustatus2 = U_ZERO_ERROR;


I think you can just use a single status variable that you reuse all the time, that makes things a bit less complex.

nielsdos · 2025-04-19T23:45:39Z

ext/intl/grapheme/grapheme_string.c

+
+	unsigned char u_break_iterator_buffer1[U_BRK_SAFECLONE_BUFFERSIZE];
+	unsigned char u_break_iterator_buffer2[U_BRK_SAFECLONE_BUFFERSIZE];
+	bi1 = grapheme_get_break_iterator((void*)u_break_iterator_buffer1, &ustatus1);


You also don't need the void* cast, you can remove that.

nielsdos · 2025-04-19T23:46:00Z

ext/intl/grapheme/grapheme_string.c

+	if (U_FAILURE(ustatus1)) {
+		intl_error_set_code(NULL, ustatus1);
+
+		intl_error_set_custom_msg(NULL, "Error on ubrk_setText on ustring1", 0);


The error message could probably be improved (same below)
e.g. "Error on ubrk_setText for argument #1 ($string1)"

Measure levenshtein for grapheme cluster unit

github-actions bot added the Extension: intl label Mar 16, 2025

youkidearitai added RFC and removed Extension: intl labels Mar 16, 2025

TimWolla changed the title ~~[Require RFC][Draft] Add grapheme_levenshtein function.~~ Add grapheme_levenshtein function. Mar 16, 2025

youkidearitai marked this pull request as ready for review April 16, 2025 00:41

youkidearitai requested review from kocsismate and devnexen as code owners April 16, 2025 00:41

youkidearitai force-pushed the grapheme_levenshtein branch from dd9a015 to 5fc7475 Compare April 16, 2025 06:09

github-actions bot added the Extension: intl label Apr 16, 2025

devnexen reviewed Apr 16, 2025

View reviewed changes

ext/intl/grapheme/grapheme_string.c Show resolved Hide resolved

devnexen approved these changes Apr 16, 2025

View reviewed changes

DanielEScherzer reviewed Apr 16, 2025

View reviewed changes

devnexen reviewed Apr 17, 2025

View reviewed changes

ext/intl/tests/grapheme_levenshtein.phpt Outdated Show resolved Hide resolved

nielsdos requested changes Apr 18, 2025

View reviewed changes

nielsdos requested changes Apr 19, 2025

View reviewed changes

youkidearitai and others added 10 commits April 21, 2025 12:10

Add grapheme_levenshtein function.

eb5f216

Measure levenshtein for grapheme cluster unit

Update for coding standards.

f22d76e

Fix some nits

c019d07

Add test for corner case

2881b1c

Fix test for output of error message

8dc01ca

grapheme_levenshtein unify internal character is UTF-16

a37e54f

Add free if U_FAILED

b88d993

Remove null check

f709ba7

Fix while

e291278

Fix usearch to ucol

6f41f98

youkidearitai force-pushed the grapheme_levenshtein branch from 1da6e4b to 6f41f98 Compare April 21, 2025 03:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add grapheme_levenshtein function. #18087

Add grapheme_levenshtein function. #18087

youkidearitai commented Mar 16, 2025

youkidearitai commented Apr 16, 2025

devnexen commented Apr 16, 2025

youkidearitai commented Apr 16, 2025

devnexen commented Apr 16, 2025

devnexen left a comment

DanielEScherzer Apr 16, 2025

youkidearitai Apr 17, 2025

youkidearitai commented Apr 17, 2025

nielsdos Apr 17, 2025

nielsdos Apr 19, 2025

nielsdos Apr 19, 2025

nielsdos Apr 18, 2025

nielsdos Apr 19, 2025

nielsdos left a comment

nielsdos Apr 19, 2025

youkidearitai Apr 20, 2025

youkidearitai Apr 21, 2025

nielsdos Apr 19, 2025

nielsdos Apr 19, 2025

nielsdos Apr 19, 2025

nielsdos Apr 19, 2025

nielsdos Apr 19, 2025

Add grapheme_levenshtein function. #18087

Are you sure you want to change the base?

Add grapheme_levenshtein function. #18087

Conversation

youkidearitai commented Mar 16, 2025

youkidearitai commented Apr 16, 2025

devnexen commented Apr 16, 2025

youkidearitai commented Apr 16, 2025

devnexen commented Apr 16, 2025

devnexen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youkidearitai commented Apr 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nielsdos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment