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.

@saghul saghul mentioned this pull request May 5, 2026
@houd1ni
Copy link
Copy Markdown

houd1ni commented May 5, 2026

Could you please share some good merged PR or a commit with similar stuff added to compare for better understanding ? ❤️

@saghul
Copy link
Copy Markdown
Contributor Author

saghul commented May 5, 2026

Not sure what you mean, can you rephrase?

This is the implementation itself, there is no other.

Ben made an inception PR but it didn't include the new syntax.

This PR implements the entire spec.

@houd1ni
Copy link
Copy Markdown

houd1ni commented May 5, 2026

can you rephrase?

I mean not about this exact spec, but another apriori well implemented. for keywords for or const or something like that where one can see the entire pipeline from keyword register in ast (like here) to its implementation itself. if nothing comes in mind, nevermind - I just found the implementation almost got it already. new noob questions:

  • what are gen/* sources ? (they misled me in the search of impl itself in the commit)
  • why one large source quickjs.c ? to avoid using Make or what ? I've been using sqlite, and the onefile-approach has been used for ease of amalgamation, and they generate it afaik.

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.

3 participants