Skip to content

Conversation

@akrieger
Copy link

@akrieger akrieger commented Nov 3, 2025

This allows C objects to be attached to functions, and it also allows C code to be notified when these functions are finalized.

Import of https://github.com/tbluemel/quickjscpp/blob/98bf1ddb00e39d93840c4b67211c9260943b5a83/patches/2024-01-13/0001-Add-C-Closures.patch. Trivial renaming of functions and function annotations to match local style.

Closes #1212. Closes tbluemel/quickjscpp#3.

@saghul
Copy link
Contributor

saghul commented Nov 4, 2025

Thank you! Could you please add a test to api-test.c?

quickjs.c Outdated
Comment on lines 5600 to 5602
JSValue JS_NewCClosure(JSContext *ctx, JSCClosure *func,
int length, int magic, void *opaque,
void (*opaque_finalize)(void*))
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with e.g. JS_SetModuleLoaderFunc:

Suggested change
JSValue JS_NewCClosure(JSContext *ctx, JSCClosure *func,
int length, int magic, void *opaque,
void (*opaque_finalize)(void*))
JSValue JS_NewCClosure(JSContext *ctx, JSCClosure *func,
void (*opaque_finalize)(void*),
int length, int magic, void *opaque)

And I'd add a typedef for the finalizer callback.

Copy link
Author

Choose a reason for hiding this comment

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

I'll defer to your judgement here, but after implementing it, it feels awkward to separate the finalizer from the data pointer. Other apis which accept opaque pointers do not have to manage the lifecycle of that pointer. Keeping the opaque_finalize argument last keeps the first 5 arguments consistent with the related JS_CFunction* apis.

Copy link
Author

@akrieger akrieger Nov 10, 2025

Choose a reason for hiding this comment

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

JS_EXTERN JSValue JS_NewCFunctionData2(JSContext *ctx, JSCFunctionData *func,
                                       const char *name,
                                       int length, int magic, int data_len,
                                       JSValueConst *data);
typedef void JSCClosureFinalizerFunc(void*);
JS_EXTERN JSValue JS_NewCClosure(JSContext *ctx, JSCClosure *func,
                                 const char* name,
                                 int length, int magic,
                                 void* opaque,  JSCClosureFinalizerFunc *opaque_finalize);

@akrieger akrieger marked this pull request as draft November 5, 2025 22:24
@akrieger akrieger marked this pull request as ready for review November 10, 2025 05:06
@akrieger
Copy link
Author

akrieger commented Nov 10, 2025

Oops forgot the test.

@akrieger akrieger marked this pull request as draft November 10, 2025 05:14
@akrieger akrieger marked this pull request as ready for review November 10, 2025 05:45
@akrieger
Copy link
Author

Hello. Anything I can do to help this be reviewed?

This allows C objects to be attached to functions, and it also
allows C code to be notified when these functions are finalized.

Co-Authored-By: Andrew Krieger <akrieger@users.noreply.github.com>
@akrieger akrieger marked this pull request as draft November 18, 2025 03:36
@akrieger akrieger marked this pull request as ready for review November 18, 2025 03:43
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.

LGTM with a question.

@saghul
Copy link
Contributor

saghul commented Nov 18, 2025

There is a failing test, which seems relevant: https://github.com/quickjs-ng/quickjs/actions/runs/19453323256/job/55673631387#step:3:1

@akrieger akrieger force-pushed the closures branch 2 times, most recently from 8a416e5 to 9da2fc4 Compare November 19, 2025 04:01
@akrieger
Copy link
Author

I really should find a way to build these myself with a more strictly conforming compiler... apologies for the back and forth.

- Add typedef for JSCClosureFinalizerFunc
- Add `name` parameter to JS_NewCClosure
- Wrap closure invocation with temporary JS stack frame for debugging.
- Fix finalizer signature to be const correct.
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.

Intent to port patches to quickjs-ng Add support for bound functions with user data pointers

4 participants