quickjs-ng / quickjs

QuickJS, the Next Generation: a mighty JavaScript engine
https://quickjs-ng.github.io/quickjs/
MIT License
967 stars 79 forks source link

About to create a external iterator for array and object #408

Closed Icemic closed 3 months ago

Icemic commented 4 months ago

I'm in the midst of implementing an external iterator for Arrays and Objects in JavaScript. As I'm diving into the code, I've noticed a couple of areas where we could use some enhancements by adding a few methods.

For Objects: When working with Objects, I've been relying on JS_GetOwnPropertyNames(JSContext *ctx, JSPropertyEnum **ptab, uint32_t *plen, JSValue obj, int flags) to fetch the property list. However, there's currently no way to clean up the memory for ptab once we're done. So, I propose adding a method like this:

void JS_FreePropertyEnum(JSContext* ctx, JSPropertyEnum* tab, uint32_t len) {
    js_free_prop_enum(ctx, tab, len);
}

For Arrays: Now, when it comes to Arrays, there's no direct method to retrieve the array's length without accessing the length property by string. It would be beneficial to introduce a method like this:

int JS_GetLength(JSContext* ctx, int64_t* pres, JSValueConst obj) {
    return js_get_length64(ctx, pres, obj);
}

And I've tested them using valgrind, no memory leak remained.

If it is ok, I'll make a pull request. Let me know if there's anything else you'd like to adjust!

saghul commented 4 months ago

I am supportive of both changes.

For the latter we can either call it JS_ArrayGetLength is it will only work for arrays, but if we make it work with typed arrays and the like we can go with your proposal.

Icemic commented 4 months ago

if we make it work with typed arrays

You're right, I forgot about the typed array. Just had a quick look, js_get_length64 seems to work for arrays, strings, functions, and arguments, which are backended by JS_ATOM_length. And typed arrays have a completely separate implementation. I'm not sure if there are any other oversights.

For typed arrays, there's a method called js_typed_array_get_length that can do the job. However, since they have entirely different implementations, perhaps having a generic JS_GetLength isn't such a good idea.

Is there a term to summarize the collection of arrays, strings, functions, and arguments? If not, I think JS_ArrayGetLength is the best choice.

chqrlie commented 4 months ago

JS_GetLength(ctx, obj) seems a good choice for a generic wrapper around JS_GetPropertyString(ctx, obj, "length"). If the length property is not an integer, throwing an exception and returning -1 seems consistent.

Icemic commented 4 months ago

a generic wrapper around JS_GetPropertyString(ctx, obj, "length")

In fact that's what I want to avoid (using JS_GetPropertyString and c string literal). It will cause overhead especially in FFI situation.

After seeing your comment in PR, I think the main point at the moment is naming?

chqrlie commented 4 months ago

a generic wrapper around JS_GetPropertyString(ctx, obj, "length")

In fact that's what I want to avoid (using JS_GetPropertyString and c string literal). It will cause overhead especially in FFI situation.

After seeing your comment in PR, I think the main point at the moment is naming?

Sorry about the confusion, I meant *as an efficient replacement of JS_GetPropertyString(ctx, obj, "length") :) Yes, naming and consistency.

Icemic commented 4 months ago

There seems to be two chioces now:

  1. A function for anything but typed arrays, but we need a proper name.
  2. Adopts JS_GetLength proposal and take account of typed arrays. (Maybe check by class_id? Hence we need to add something like JS_IsTypedArray at first...maybe a long way)

Any suggestions? Which one we should choose to go?

chqrlie commented 4 months ago

There seems to be two chioces now:

  1. A function for anything but typed arrays, but we need a proper name.
  2. Adopts JS_GetLength proposal and take account of typed arrays. (Maybe check by class_id? Hence we need to add something like JS_IsTypedArray at first...maybe a long way)

Any suggestions? Which one we should choose to go?

js_get_length64 works for typed arrays too as they have an accessor for the length property. The implementation could be optimized, but already works. So I would recommend you use JS_GetLength() and implement it as a simple wrapper around js_get_length64.

If the argument order stays the same, it would be consistent with JS_GetArrayBuffer but not with JS_GetTypedArrayBuffer. I would prefer that the object be passed first after the context for published APIs, what do you think @saghul ?

Icemic commented 4 months ago

js_get_length64 works for typed arrays too

Oh, I think I got something wrong from the beginning. I haven't been able to find out how this function gets the length of typed arrays until now, but I wrote a little demo and it does work. Maybe I should have done this from the beginning :(

Fortunately things are so much simpler now: A int JS_GetLength(JSContext *ctx, int64_t *pres, JSValue obj) or int JS_GetLength(JSContext *ctx, JSValue obj, int64_t *pres) will be introduced.

I would prefer that the object be passed first after the context for published APIs

I vote for this.

saghul commented 4 months ago

If the argument order stays the same, it would be consistent with JS_GetArrayBuffer but not with JS_GetTypedArrayBuffer. I would prefer that the object be passed first after the context for published APIs, what do you think @saghul ?

100%, context first.

chqrlie commented 4 months ago

int JS_GetLength(JSContext ctx, JSValue obj, int64_t pres)

For the avoidance of doubt, this means:

int JS_GetLength(JSContext *ctx, JSValue obj, int64_t *pres);
Icemic commented 3 months ago

Solved #409