Skip to content

Implement explicit resource management#1458

Draft
saghul wants to merge 1 commit intomasterfrom
explicit-resource-management
Draft

Implement explicit resource management#1458
saghul wants to merge 1 commit intomasterfrom
explicit-resource-management

Conversation

@saghul
Copy link
Copy Markdown
Contributor

@saghul saghul commented Apr 22, 2026

Co-authored-by: Ben Noordhuis info@bnoordhuis.nl

Closes: #865

@saghul saghul force-pushed the explicit-resource-management branch from a8b475d to 42bc792 Compare April 22, 2026 08:49
Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>

Closes: #865
@saghul saghul force-pushed the explicit-resource-management branch from 42bc792 to 4fdbe0e Compare April 22, 2026 11:46
Copy link
Copy Markdown
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

+1700 lines is more than I have time to review in-depth at the moment. I'll trust your good judgement.

Comment thread quickjs-opcode.h
Otherwise looks up Symbol.asyncDispose (with Symbol.dispose fallback
when u8=1) or Symbol.dispose (u8=0) and throws TypeError if missing
or non-callable. Stack: value -> value, method. */
DEF( using_check, 2, 1, 2, u8)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

AI comments, right? I'd remove them and just let the code do the talking (in general, not just here; I already spot several in quickjs.c)

Rule of thumb: comment why, not what, and only when the why is not obvious when looking at the code itself.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yep, and sure thing, I opened as draft do a few more passes on it.

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