Skip to content

[RFC] Add grapheme_strrev function#20949

Open
youkidearitai wants to merge 6 commits intophp:masterfrom
youkidearitai:grapheme_strrev
Open

[RFC] Add grapheme_strrev function#20949
youkidearitai wants to merge 6 commits intophp:masterfrom
youkidearitai:grapheme_strrev

Conversation

@youkidearitai
Copy link
Contributor

Add grapheme_strrev function. strrev for grapheme cluster unit.

RFC: https://wiki.php.net/rfc/grapheme_strrev

Add more tests Arabic for grapheme_strrev function.
@youkidearitai youkidearitai marked this pull request as ready for review February 20, 2026 01:47
@youkidearitai
Copy link
Contributor Author

This RFC is accepted, So ready for review.

@youkidearitai
Copy link
Contributor Author

There's no rush, Does anyone review this?

p = ZSTR_VAL(ret);

ubrk_setUText(bi, ut, &ustatus);
pos = ubrk_last(bi);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might be worth check in case pos == UBRK_DONE is to "jump to conclusion" to bypass the loop below wdyt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I added RETVAL_EMPTY_STRING.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one detail, do not forget you allocate ret pointer line 1172.

Copy link
Member

@devnexen devnexen Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might just simpler to raise the goto label at the 1190 line instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried add ubrk_end in RETVAL_NEW_STR .

ubrk_setUText(bi, ut, &ustatus);
pos = ubrk_last(bi);
if (pos == UBRK_DONE) {
RETVAL_EMPTY_STRING();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but then this line looks useless wdyt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I remove RETVAL_EMPTY_STRING.

Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LCTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants