-
Notifications
You must be signed in to change notification settings - Fork 824
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
Add types and methods to provide updated API #1313
Conversation
We now use `,*` on the outside of a `$()` expression to match on interspersed commas instead of trailing commas. To continue to handle the trailing comma case, we optionally match on an extra comma at the end with `$(,)?`.
1e263c7
to
71be2c6
Compare
/// Get an export. | ||
/// | ||
/// ``` | ||
/// # use wasmer_runtime_core::{DynFunc, Func, Instance}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be wasmer_runtime
instead? I would prefer people depending on the the outer shell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's possible to move the doc comments with examples to wasmer_runtime
instead, though the #
on that line means that it's invisible to users in the docs, so it doesn't matter here
This commit also leaves comments explaining the current state of things so that when it's unblocked it can be finished and the API made public.
} | ||
|
||
// copied from Rust stdlib: https://doc.rust-lang.org/nightly/nightly-rustc/src/serialize/leb128.rs.html#4-14 | ||
macro_rules! impl_write_unsigned_leb128 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not strictly needed, but in case we needed the other functions, I figured it might be good to have around. I can macro-expand/inline the one function I need for the custom section writing if you'd prefer that!
Can you make |
bors try |
tryBuild failed |
bors try |
tryBuild failed |
tryTimed out |
Deprecate more methods on `Instance`, add `into_iter` method on `Exports`, add FuncSig to ImportType and other updates.
bors try |
tryBuild succeeded |
bors try |
tryBuild failed |
bors try |
@@ -716,10 +716,19 @@ impl LocalGlobal { | |||
} | |||
|
|||
/// Identifier for a function signature. | |||
/// | |||
/// A transparent `SigIndex` | |||
#[derive(Debug, Clone, Copy)] | |||
#[repr(transparent)] | |||
pub struct SigId(pub u32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we replace all uses of SigId
with SigIndex
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that it's transparent
may be important to its use. I just added the comment because I get confused every time I see it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to verify that we are making use of transparent
(kind of: tests without it will pass?)
If tests pass, then we can just remove this type and use SigIndex directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's stored by pointer and might be accessed by the generator code, having it not be transparent could mean that depending on the optimization level, for example, it might just corrupt the data that generated code sees. It is possible to have tests for this, but it's probably not the simplest thing to test. I don't really know how it's used, so maybe it's easier to test than I think it is.
tryBuild succeeded |
lib/runtime-core/src/module.rs
Outdated
/// The name identifying the export. | ||
pub name: &'a str, | ||
/// The type of the export. | ||
pub ty: ExportType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have here ty: ExternType
?
bors try |
bors try- |
bors r+ |
Build succeeded
|
Improving the API in a number of ways.
Current status
wrap
being not linear time update (this update should also make it possible to retrieve aFunc
fromvm::Anyfunc
)Review