Skip to content

feat(ecmascript): %TypedArray%.prototype.slice #736

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

yossydev
Copy link
Contributor

@yossydev yossydev force-pushed the feat/typedarray-prototype-slice branch from fa5ab1e to 710456b Compare May 25, 2025 09:10
@yossydev yossydev marked this pull request as ready for review May 25, 2025 09:44
@yossydev yossydev force-pushed the feat/typedarray-prototype-slice branch from e16349a to 555ae9c Compare May 31, 2025 22:53
@yossydev yossydev requested a review from aapoalas May 31, 2025 22:54
@yossydev yossydev force-pushed the feat/typedarray-prototype-slice branch from 555ae9c to 5a70dcb Compare May 31, 2025 23:13
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.

Some nitpicks and one or two issues, that may actually be non-issues?

But overall, looks really good! Thank you, and great work <3

.unbind()?
.bind(gc.nogc());
let o = ta_record.object;
// 3. Let srcArrayLength be TypedArrayLength(taRecord).
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: This comment should be inside the slice_typed_array method.

gc: GcScope<'gc, '_>,
) -> JsResult<'gc, Value<'gc>> {
Err(agent.todo("TypedArray.prototype.slice", gc.into_nogc()))
let start = arguments_list.get(0);
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: Both the get call results and the o = this_value; should have a .bind(gc.nogc()) attached. Here it does not make a difference, but it's better to have them here just so that anyone coming in later and using this method as a template to copy-paste from will have the right "idioms" in place.

let end = end.scope(agent, gc.nogc());
let src_array_length = typed_array_length::<T>(agent, &ta_record, gc.nogc()) as i64;
// 4. Let relativeStart be ? ToIntegerOrInfinity(start).
let relative_start = to_integer_or_infinity(agent, start.get(agent), gc.reborrow()).unbind()?;
Copy link
Member

Choose a reason for hiding this comment

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

issue: No reason to scope start as it is accessed here directly after. You can just remove the scoping line and change this to use start.unbind():

Suggested change
let relative_start = to_integer_or_infinity(agent, start.get(agent), gc.reborrow()).unbind()?;
let relative_start = to_integer_or_infinity(agent, start.unbind(), gc.reborrow()).unbind()?;

let end = end.bind(gc.nogc());
let o = ta_record.object;
let o = o.scope(agent, gc.nogc());
let start = start.scope(agent, gc.nogc());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let start = start.scope(agent, gc.nogc());

relative_start.into_i64().min(src_array_length)
};
// 8. If end is undefined, let relativeEnd be srcArrayLength; else let relativeEnd be ? ToIntegerOrInfinity(end).
let end_index = if end.get(agent).is_undefined() {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: You could take end out of the scoping here:

Suggested change
let end_index = if end.get(agent).is_undefined() {
// SAFETY: end is not shared.
let end = unsafe { end.take(agent) }.bind(gc.nogc());
let end_index = if end.is_undefined() {

src_array_length
} else {
let integer_or_infinity =
to_integer_or_infinity(agent, end.get(agent), gc.reborrow()).unbind()?;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
to_integer_or_infinity(agent, end.get(agent), gc.reborrow()).unbind()?;
to_integer_or_infinity(agent, end.unbind(), gc.reborrow()).unbind()?;

.unbind()?
.bind(gc.nogc());
// 14. If countBytes > 0, then
if count_bytes > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: Could maybe return early if count_bytes == 0 and de-indent the if-branch.

if is_typed_array_out_of_bounds::<T>(agent, &ta_record, gc.nogc()) {
return Err(agent.throw_exception_with_static_message(
ExceptionType::TypeError,
"Callback is not callable",
Copy link
Member

Choose a reason for hiding this comment

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

issue: Wrong error message.

));
};
// c. Set endIndex to min(endIndex, TypedArrayLength(taRecord)).
// let end_index = end_index.min(typed_array_length::<T>(agent, &ta_record, gc.nogc()) as i64);
Copy link
Member

Choose a reason for hiding this comment

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

issue: Commented out code. Are these being done inside the with_typed_array_viewable!(a, ...) branch? If so, the comments should be moved there. Otherwise, the code should presumably be actually performed.

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