Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify exiting interpreter with exception #789

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Jan 6, 2025

No description provided.

@saghul saghul force-pushed the simplify-exception-exit branch 2 times, most recently from 6a0624a to 196c0b4 Compare January 6, 2025 10:25
@saghul saghul marked this pull request as draft January 8, 2025 12:41
@saghul
Copy link
Contributor Author

saghul commented Jan 8, 2025

I just had an idea change things a bit more, will post an update.

- Avoid keeping the exception object around
- Avoid passing the responsibility of freeing the exeption object to the
  caller
@saghul saghul force-pushed the simplify-exception-exit branch from 196c0b4 to 1bf7525 Compare January 8, 2025 15:18
@saghul saghul requested a review from bnoordhuis January 8, 2025 15:18
@saghul saghul marked this pull request as ready for review January 8, 2025 15:18
@saghul
Copy link
Contributor Author

saghul commented Jan 8, 2025

@bnoordhuis PTAL, I think it's a cleaner implementation now, and more "backwars compatible" as in, code that calls js_std_loop won't leak any object in case of exception.

js_std_free_handlers(rt);
JS_FreeContext(ctx);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reversed the order here since I noticed that's how qjs.c does it.

@saghul saghul merged commit 97ea19d into master Jan 8, 2025
59 checks passed
@saghul saghul deleted the simplify-exception-exit branch January 8, 2025 20:37
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