Skip to content

Add new Wasmtime::Extern class#14

Merged
ianks merged 8 commits intomainfrom
wrapped-struct
Nov 15, 2022
Merged

Add new Wasmtime::Extern class#14
ianks merged 8 commits intomainfrom
wrapped-struct

Conversation

@ianks
Copy link
Copy Markdown
Collaborator

@ianks ianks commented Nov 13, 2022

Previously, Extern was implicitly mapped to the underlying variant. For example, a Extern::Func would automatically become Wasmtime::Func when loading the extern in Ruby. This differs from Wasmtime, where the caller must do the conversion to ensure the export they want is valid.

To accomplish this, I added a WrappedStruct<T> helper to codify a bunch of the conversions we were doing previously. This cleans thing up nicely, so I made a proposal to magnus for it as well.

@ianks ianks requested a review from jbourassa November 13, 2022 05:30
Copy link
Copy Markdown
Collaborator

@jbourassa jbourassa left a comment

Choose a reason for hiding this comment

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

This differs from Wasmtime, where the caller must do the conversion to ensure the export they want is valid.

I also want to stay close to Wasmtime's API (see #13 (comment)), but in this particular case I didn't see the benefits. Can you expand on why you think the wrapper approach is preferable?

I assumed this was done this way in Wasmtime because of static typing, not ergonomics. The no wrapper version feels more idiomatic Ruby to me. For what it's worth, the Python bindings also return the extern directly, without any wrapper.

phantom: PhantomData,
})
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👌 love this.

@ianks
Copy link
Copy Markdown
Collaborator Author

ianks commented Nov 13, 2022

This differs from Wasmtime, where the caller must do the conversion to ensure the export they want is valid.

I also want to stay close to Wasmtime's API (see #13 (comment)), but in this particular case I didn't see the benefits. Can you expand on why you think the wrapper approach is preferable?

I assumed this was done this way in Wasmtime because of static typing, not ergonomics. The no wrapper version feels more idiomatic Ruby to me. For what it's worth, the Python bindings also return the extern directly, without any wrapper.

My thinking is that the implicit behavior could cause some strange, potentially undesirable behavior. For example, both Global and Table expose get and set methods. There may be unintended consequences of calling set on a Global, on what you assumed to have been a table.

There's also Memory and SharedMemory, and potentially other overlapping interfaces... By going through an Extern, we ensure the user indicates what they expect the underlying extern type to be. So in this case, my intuition is to default to the "correct" rather than "slightly more convenient." If this is burdensome to users, it's easy enough to add a helper to do the implicit unwrap thing. Not so easy to go the other way...

@jbourassa
Copy link
Copy Markdown
Collaborator

My thinking is that the implicit behavior could cause some strange, potentially undesirable behavior.

I think that makes sense. The price to pay is quite small: typing to_..., and 1 extra object allocation. :shipit: and I'll rebase #2 on top of it.

@@ -89,15 +93,15 @@ impl Instance {
/// @def exports
/// @return [Hash{String => Func, Memory}]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We're gonna have to update that signature, along with Instance#export and Linker#get.

I can take care of it once it's merged though.

@ianks ianks requested a review from jbourassa November 15, 2022 17:53
@ianks ianks merged commit 8b531c9 into main Nov 15, 2022
@ianks ianks deleted the wrapped-struct branch November 15, 2022 18:53
jbourassa added a commit that referenced this pull request Nov 16, 2022
jbourassa added a commit that referenced this pull request Nov 16, 2022
jbourassa added a commit that referenced this pull request Nov 16, 2022
@jbourassa jbourassa mentioned this pull request Nov 16, 2022
jbourassa added a commit that referenced this pull request Nov 16, 2022
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