Skip to content

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.

@saghul
Copy link
Contributor

saghul commented Dec 17, 2024

IsTupedArray with the enum? That works too.

@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!

@saghul
Copy link
Contributor

saghul commented Dec 21, 2024

@chqrlie Any further comments?

@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

5 participants