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

Add typed array utility functions #760

Merged
merged 5 commits into from
Dec 22, 2024

Conversation

richarddd
Copy link
Contributor

Expose a few utility functions to check and create typed arrays. Also add JS_NewTypedArray from upstream.

Fixes #758

Copy link
Collaborator

@chqrlie chqrlie left a comment

Choose a reason for hiding this comment

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

I really don't like this approach: instead of 12 more APIs, I suggest a generic extensible method.

quickjs.h Outdated Show resolved Hide resolved
quickjs.h Show resolved Hide resolved
quickjs.c Show resolved Hide resolved
quickjs.c Show resolved Hide resolved
@saghul
Copy link
Contributor

saghul commented Dec 17, 2024

IsTupedArray with the enum? That works too.

quickjs.c Show resolved Hide resolved
quickjs.c Show resolved Hide resolved
@richarddd richarddd requested a review from chqrlie December 17, 2024 15:31
@richarddd
Copy link
Contributor Author

@saghul @chqrlie OK to merge now?

Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

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

One minor nit, LGTM otherwise!

quickjs.c Show resolved Hide resolved
@saghul
Copy link
Contributor

saghul commented Dec 21, 2024

@chqrlie Any further comments?

quickjs.c Outdated Show resolved Hide resolved
@chqrlie
Copy link
Collaborator

chqrlie commented Dec 21, 2024

@chqrlie Any further comments?

Aside from the remark about the obscure code in JS_GetTypedArrayType, there is a gotcha in the published enum: JS_TYPED_ARRAY_FLOAT16 in the published API is before JS_TYPED_ARRAY_FLOAT32, which seems logical, but makes the last 2 enum values incompatible with the enum published in bellard/quickjs.

If we keep this logical ordering, I will have to break backward compatibility when backporting the float16 support.

I cannot measure if this may be a problem or not, @saghul and @bnoordhuis what is your experience?

@saghul
Copy link
Contributor

saghul commented Dec 21, 2024

I'd say that's ok but I wouldn't mind moving it around either.

@chqrlie
Copy link
Collaborator

chqrlie commented Dec 22, 2024

I'd say that's ok but I wouldn't mind moving it around either.

@bnoordhuis what is your take on this?

@bnoordhuis
Copy link
Contributor

I'd say it's fine to keep it the way it is. We're not promising AP/ABI stability just yet.

@saghul saghul merged commit 7f156b6 into quickjs-ng:master Dec 22, 2024
59 checks passed
@richarddd richarddd deleted the feat/typed-array-functions branch December 22, 2024 18:21
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.

Expose predefined class_ids for efficient lookup?
5 participants