-
Notifications
You must be signed in to change notification settings - Fork 38
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
capi: imported and exported memory #631
Conversation
645d83f
to
f848f1b
Compare
Codecov Report
@@ Coverage Diff @@
## master #631 +/- ##
==========================================
+ Coverage 98.36% 98.37% +0.01%
==========================================
Files 69 69
Lines 9592 9654 +62
==========================================
+ Hits 9435 9497 +62
Misses 157 157 |
f848f1b
to
e89ffb5
Compare
036b86a
to
d8a6174
Compare
e89ffb5
to
d06b3be
Compare
d06b3be
to
e2350d8
Compare
aca8ba0
to
3b51d9f
Compare
e2350d8
to
cd307cc
Compare
@@ -190,8 +204,8 @@ bool fizzy_find_exported_function( | |||
/// module, behaviour is undefined. | |||
FizzyInstance* fizzy_instantiate(const FizzyModule* module, | |||
const FizzyExternalFunction* imported_functions, size_t imported_functions_size, | |||
const FizzyExternalTable* imported_table, const FizzyExternalGlobal* imported_globals, | |||
size_t imported_globals_size); | |||
const FizzyExternalTable* imported_table, const FizzyExternalMemory* imported_memory, |
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 C++ API supports a vector of tables and memories, while the C API only supports a singular for both. While both can be at most one in wasm 1.0, there is a discrepancy between the two APIs.
Perhaps should also comment about this, as it may be confused to those familiar with the wasm spec.
In short: I think we should support a single table/memory in the C API as we can make a breaking API release if we do start supporting newer wasm versions. I would be inclined to say the same restriction would make sense in C++. Perhaps just open a tracking issue about this?
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.
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.
I agree on having single table/memory.
include/fizzy/fizzy.h
Outdated
/// @param name The table name. NULL-terminated string. Cannot be NULL. | ||
/// @param out_memory Pointer to output struct to store found memory. Cannot be NULL. | ||
/// @returns true if memory was found, false otherwise. | ||
|
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 leave a comment this support a singular exported memory as per wasm 1.0.
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.
Well this function would support several memories just fine, because it finds them by name.
include/fizzy/fizzy.h
Outdated
{ | ||
// Opaque pointer to memory data. | ||
FizzyMemory* memory; | ||
// Memory limits. |
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.
// Memory limits. | |
/// Memory limits. |
6c42874
to
5293b84
Compare
5293b84
to
773ec2a
Compare
@@ -190,8 +204,8 @@ bool fizzy_find_exported_function( | |||
/// module, behaviour is undefined. | |||
FizzyInstance* fizzy_instantiate(const FizzyModule* module, | |||
const FizzyExternalFunction* imported_functions, size_t imported_functions_size, | |||
const FizzyExternalTable* imported_table, const FizzyExternalGlobal* imported_globals, | |||
size_t imported_globals_size); | |||
const FizzyExternalTable* imported_table, const FizzyExternalMemory* imported_memory, |
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.
I agree on having single table/memory.
No description provided.