Skip to content

[Strings] Add experimental StringNew variants #5459

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

Merged
merged 13 commits into from
Jan 27, 2023
Merged

[Strings] Add experimental StringNew variants #5459

merged 13 commits into from
Jan 27, 2023

Conversation

kripken
Copy link
Member

@kripken kripken commented Jan 26, 2023

string.from_code_point makes a string from an int code point.

string.new_utf8*_try makes a utf8 string and returns null on a UTF8 encoding
error rather than trap.

@kripken kripken requested a review from tlively January 26, 2023 20:50
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

LGTM % comments

code == BinaryConsts::StringNewUTF8Try) {
if (code == BinaryConsts::StringNewUTF8Try) {
try_ = true;
}
if (getInt8() != 0) {
Copy link
Member

Choose a reason for hiding this comment

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

This is existing code, but surely the memory index should be a ULEB32 rather than a byte?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I think you're right, I'll add a TODO here. When we implement multimemory support for these we should fix that.

Comment on lines 1 to 2
;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up.
Copy link
Member

Choose a reason for hiding this comment

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

Is this a mistake?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's intentional - this test now has a manual CHECK-NOT so it can't be auto-updated (the update removes that line!). I missed this when landing and it's annoying in local work. I can open a separate PR, but it is trivial?

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved out.

@kripken kripken merged commit 0e4712f into main Jan 27, 2023
@kripken kripken deleted the string.news branch January 27, 2023 00:11
kripken pushed a commit that referenced this pull request Feb 1, 2023
Adds APIs for string.from_code_point, string.new_utf8_try,
string.new_utf8_array_try (#5459) and string.compare (#5453).
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