Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sv_vcatpvfn_flags: Use utf8_to_uv_or_die #23083

Open
wants to merge 1 commit into
base: blead
Choose a base branch
from

Conversation

khwilliamson
Copy link
Contributor

This is a change in behavior for malformed input. Previously it warned and substituted 0; the warnings remain, but now it substitutes the REPLACEMENT CHARACTER.

  • This set of changes does not require a perldelta entry.

@tonycoz
Copy link
Contributor

tonycoz commented Mar 10, 2025

This is a change in behavior for malformed input. Previously it warned and substituted 0; the warnings remain, but now it substitutes the REPLACEMENT CHARACTER.

I'm confused, you're calling utf8_to_uv_or_die(), which croaks on a decoding failure, but the commit message says it produces a replacement character instead.

This is a change in behavior for malformed input.  Previously it warned
and substituted 0; the warnings remain, but now it croaks.
@khwilliamson
Copy link
Contributor Author

It looks like I was the one who was confused. I hope it was just a copy paste error. Anyway, it doesn't return, but croaks in the face of malformed input. I'm now a bit reticent about doing this given that a couple of such commits had to be backed out. But I don't know that there is a decent recovery path forward here when this condition occurs. And there are already places in this function where it panics when it doesn't know how to go on

@tonycoz
Copy link
Contributor

tonycoz commented Mar 10, 2025

  • This set of changes does not require a perldelta entry.

If this and other similar changes remain I do think we do need some sort of perldelta entry.

@khwilliamson
Copy link
Contributor Author

I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants