Skip to content

feat: Add JSTypedArray type #26

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 5 commits into from
Nov 2, 2023
Merged

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Oct 30, 2023

This PR should ideally be reviewed commit-by-commit.

It introduces the JSTypedArray type, and a few other methods like JSValue::is_typed_array and JSValue::as_typed_array. The most important bit is probably JSTypedArray::as_mut_slice.

Happy to get your feedback.

Hywan added 2 commits October 30, 2023 12:33
This patch implements `JSValue::get_typed_array_type` to know the type
of a typed array.
This patch implements `JSValue::is_typed_array` on top of
`JSValue::get_typed_array_type`.
This patch introduces the `JSTypedArray` type. It holds the `ty` method
(which replaces `JSValue::get_typed_array_type`), `len`, `byte_offset`,
`byte_length` and `as_mut_slice`.
Copy link
Contributor

@waywardmonkeys waywardmonkeys 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 so far. Left comments with tweaks… might try to recruit someone to do a safety review, but not necessarily before merging it…

///
/// # Safety
///
/// Ensure `raw` is valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe also say it must be a typed array? Or can it reasonably be something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the doc to say it must be a typed array, but the C API is defensive: if the JSObjectRef isn't a typed array, it shouldn't crash. But still, it must be a JSObjectRef of a typed array.

}

/// Returns a value of type [`JSTypedArrayType`] that identifies value's
/// Typed Array type, or `JSTypedArrayType::None` if the value is not a Typed Array
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this returns a result type, what could, the error be? And can this ever contain something that isn’t a typed array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JSTypedArrayType::None represents more or less the error case.

/// .unwrap();
/// assert_eq!(array.len().unwrap(), 5);
/// ```
#[allow(clippy::len_without_is_empty)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Just add an is_empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure it was making sense to have TypedArray::is_empty, because all items exists by definition, just with a default value, so a typed array is never empty.

@waywardmonkeys
Copy link
Contributor

@Hywan Just checking in on this ...

@Hywan
Copy link
Contributor Author

Hywan commented Nov 2, 2023

@waywardmonkeys I've pushed my last modification.

@waywardmonkeys
Copy link
Contributor

Sounds so final! But it looks good to me.

@waywardmonkeys waywardmonkeys merged commit 5a46eb5 into endoli:main Nov 2, 2023
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