-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(jstz_engine): implement JsString
and JsStr
wrappers
#740
base: main
Are you sure you want to change the base?
Conversation
f92c260
to
fe9eb18
Compare
e053775
to
072cbc2
Compare
Codecov ReportAttention: Patch coverage is
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
fe9eb18
to
0a6202d
Compare
072cbc2
to
a33ec7b
Compare
a33ec7b
to
d929480
Compare
d929480
to
e4b8240
Compare
Need review from @johnyob |
crates/jstz_engine/src/string/mod.rs
Outdated
} | ||
|
||
/// Obtains a slice of a [`JsString`] as a [`JsStr`]. | ||
pub fn as_str<'cx, S>(&self, cx: &'cx Context<S>) -> JsStr<'cx> |
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.
Do we want &'cx Context<S>
or &'cx mut Context<S>
? The idea behind &mut Context<S>
in jstz_engine
is that it is a capability for doing anything with SpiderMonkey. But we could be more permissive and have &Context<S>
=> no gc and &mut Context<S>
=> gc
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.
Actually having looked at the code for the JS_*
functions here, we do in fact allocate if the string are not linear 😔
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.
Updated to mut Context
crates/jstz_engine/src/string/mod.rs
Outdated
|
||
impl<'a, C: Compartment> JsString<'a, C> { | ||
/// Creates a new empty [`JsString`] | ||
pub fn empty<S>(cx: &'a Context<S>) -> Self |
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.
pub fn empty<S>(cx: &'a Context<S>) -> Self | |
pub fn empty<S>(cx: &'a mut Context<S>) -> Self |
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.
done
crates/jstz_engine/src/string/mod.rs
Outdated
pub fn code_point_at<'cx, S>(&self, index: usize, cx: &'cx Context<S>) -> u16 | ||
where | ||
S: InCompartment<C> + CanAccess, | ||
'cx: 'a, | ||
{ | ||
unsafe { | ||
let mut char = 0; | ||
JS_GetStringCharAt(cx.as_raw_ptr(), self.as_raw_ptr(), index, &mut char); | ||
char | ||
} | ||
} |
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.
Unfortunately code_point_at
can allocate.
If passed a non-linear string, SpiderMonkey will use ensureLinear
, which will allocate an equivalent linear string
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.
Updated to mut Context
crates/jstz_engine/src/string/mod.rs
Outdated
S: InCompartment<C> + CanAlloc, | ||
{ | ||
unsafe { | ||
/* https://searchfox.org/mozilla-central/source/js/public/String.h#54 |
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.
We should refer to the mozjs SpiderMonkey code -- the version hosted here differs from the one we use
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.
Updated
crates/jstz_engine/src/string/mod.rs
Outdated
pub fn equals<S>( | ||
s1: &JsString<'_, C>, | ||
s2: &JsString<'_, C>, | ||
cx: &'_ Context<S>, | ||
) -> Option<bool> |
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.
Similarly, equality can allocate since it will implicitly allocate linear strings if given a non-linear string
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.
Updated to mut Context
1af8aee
to
91cce29
Compare
As per our offline discussion, we have taken the decision to always use |
91cce29
to
ce2b057
Compare
Context
Related Tasks: jstz-214
Description
Dependencies: #739
Adds type definitions for
JsString
andJsStr
.Additionally wraps most (if not all) functions that operate on strings.
Manually testing the PR