Skip to content

Conversation

@GeoffreyParrier
Copy link
Contributor

For both files, functions now returns a wtf8::CodePoint

@GeoffreyParrier
Copy link
Contributor Author

GeoffreyParrier commented Oct 14, 2025

Small questions, are these comments still relevant?

There is wt8 handling so it seems irrelevant now and could be removed but I didn't want to remove them without being sure

https://github.com/trynova/nova/blob/main/nova_vm/src/ecmascript/types/language/string.rs#L318-L319

@aapoalas
Copy link
Member

Small questions, are these comments still relevant?

There is wt8 handling so it seems irrelevant now and could be removed but I didn't want to remove them without being sure

https://github.com/trynova/nova/blob/main/nova_vm/src/ecmascript/types/language/string.rs#L318-L319

I believe there is still some relevance to those comments, although not in the way they say: small strings can be WTF-8 as well nowadays, which means that concatenating two small strings requires special work to check for cases where a pair of unpaired surrogates are concatenated into one another, and a byte marriage is required :)

In effect, that function has a bug that creates... Uuh, library level UB at least.

Copy link
Member

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Hello, and welcome to Nova! Thank you for this cleanup, this looks good to me <3

If you want to try fix the WTF-8 concatenation bug in the other function as part of this PR, that's fine by me. Otherwise, I can merge this right away.

@GeoffreyParrier
Copy link
Contributor Author

Maybe merge this and I'll take a look later, as I'm not familiar with rust or lone surrogate, I prefer not to block this PR

@aapoalas aapoalas merged commit ab81608 into trynova:main Oct 14, 2025
8 checks passed
@GeoffreyParrier GeoffreyParrier deleted the patch-2 branch October 27, 2025 07:25
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