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

Initial support for external v8 strings #641

Merged
merged 9 commits into from
Mar 7, 2021

Conversation

AaronO
Copy link
Contributor

@AaronO AaronO commented Mar 6, 2021

This PR partially fixes #636, by adding support for external static rust strings.

Rust doesn't have utf16 (or two-byte string) literals, so we could shorten the name in this static case, i.e:

v8::String::new_external_static()
instead of
v8::String::new_external_onebyte_static()

We could support non-static external strings too, but I'm not sure we want to encourage that.

Partially fixes denoland#636, by adding support for static rust strings
@AaronO AaronO changed the title Implement v8::String::new_external_onebyte_static() Initial support for external v8 strings, starting with one-byte static Mar 6, 2021
@AaronO AaronO changed the title Initial support for external v8 strings, starting with one-byte static Initial support for external v8 strings Mar 6, 2021
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Looks good. Could you add a small test?

I'd prefer to keep the function name the same as in V8, as you have done.

AaronO added 2 commits March 7, 2021 02:54
As well as is_external_onebyte() and is_external_twobyte()
@AaronO
Copy link
Contributor Author

AaronO commented Mar 7, 2021

@ry Added some simple tests and exposed some relevant v8 methods:

is_external()
is_external_onebyte()
is_external_twobyte()
is_onebyte()

See notes on is_external(), it needs to be updated on next v8 release, since v8::String::IsExternal() was only recently re-added (https://source.chromium.org/chromium/_/chromium/v8/v8.git/+/1dd8624b524d14076160c1743f7da0b20fbe68e0).

The C++ binding fallsback to v8::Value::IsExternal() which is incorrect, the rust fallback I added is perfectly functional however, so this can ship without updating v8

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thanks @AaronO

@ry ry merged commit f2766ed into denoland:main Mar 7, 2021
@AaronO AaronO deleted the strings/new-external-static branch March 7, 2021 18:39
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.

Support external v8 strings
2 participants