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

feat: wasm_name_new_from_string* owns s #155

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Hywan
Copy link
Contributor

@Hywan Hywan commented Oct 9, 2020

wasm_name_new is an alias to wasm_byte_vec_new, which owns the
ptr_or_none argument, which is in this case s.

This patch adds the own market to wasm_name_new_from_string and
wasm_name_new_from_string_nt.

`wasm_name_new` is an alias to `wasm_byte_vec_new`, which owns the
`ptr_or_none` argument, which is in this case `s`.

This patch adds the `own` market to `wasm_name_new_from_string` and
`wasm_name_new_from_string_nt`.
@rossberg
Copy link
Member

Oh, ouch. I see there is an inconsistency there. Unfortunately, I think this is the wrong fix. These constructors don't take ownership of the string in the implementation, nor should they -- you wouldn't be able to create a name form a string literal, for example.

It's the definition of WASM_DECLARE_VEC that is wrong here. The own on wasm_byte_vec_new doesn't actually make sense when ptr_or_none is empty.

I think the right fix would be to move the own from the macro's definition to its invocations, passing own* for ptr_or_none where it currently passes just *. Or to split the macro in three, analogous to the implementation in wasm-c.cc.

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