Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: dump support symbol (close: #77) #78

Closed
wants to merge 1 commit into from

Conversation

ahaoboy
Copy link

@ahaoboy ahaoboy commented Nov 4, 2022

No description provided.

@ahaoboy
Copy link
Author

ahaoboy commented Nov 4, 2022

For easy to use, I think should provide utils function dump any object from qjs and export any object to qjs

ts/context.ts Outdated
@@ -738,6 +738,9 @@ export class QuickJSContext implements LowLevelJavascriptVm<QuickJSHandle>, Disp
return this.getNumber(handle)
} else if (type === "undefined") {
return undefined
} else if (type === "symbol") {
const desc = this.getString(this.getProp(handle, "description"))
Copy link
Owner

Choose a reason for hiding this comment

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

I think the property handle is leaked. It needs to be disposed.

Copy link
Author

Choose a reason for hiding this comment

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

Fix by this?

      const propHd = this.getProp(handle, "description")
      const desc = this.getString(propHd)
      propHd.dispose()
      return Symbol(desc)

@justjake justjake enabled auto-merge (squash) February 18, 2023 22:31
@justjake justjake mentioned this pull request Feb 19, 2023
auto-merge was automatically disabled February 19, 2023 23:44

Pull request was closed

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants