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
join: extra check for empty string + empty sep #576
Conversation
this could cause \r and \n to be kept as two graphemes when they were joined with an empty string in between and the empty string as the separator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just these two things.
src/strings/ops.c
Outdated
@@ -1220,9 +1220,18 @@ MVMString * MVM_string_join(MVMThreadContext *tc, MVMString *separator, MVMObjec | |||
|
|||
/* Add separator if needed. */ | |||
if (i > 0) { | |||
/* If there's no separator and one piece is The Empty String we | |||
* have to be extra careful about concat stability */ | |||
if (sgraphs == 0 && MVM_string_graphs(tc, piece) == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use MVM_string_graphs_nocheck here since MVM_string_graphs is already run on each piece in the loop above.
src/strings/ops.c
Outdated
* have to be extra careful about concat stability */ | ||
if (sgraphs == 0 && MVM_string_graphs(tc, piece) == 0) { | ||
if (!concats_stable) | ||
; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this empty block for? I think it would be more clear if the if statement on L1225 checked concat_stable and the else if loop
below become just a plain if loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Will merge after Travis 👍
this could cause \r and \n to be kept as two graphemes
when they were joined with an empty string in between and
the empty string as the separator.