Conversation
gumb0
changed the title
Codecov Report
@@ Coverage Diff @@
## master #683 +/- ##
========================================
Coverage 99.30% 99.30%
========================================
Files 72 72
Lines 10312 10420 +108
========================================
+ Hits 10240 10348 +108
Misses 72 72
Flags with carried forward coverage won't be shown. Click here to find out more.
|
gumb0
force-pushed
the
capi-import-inspection
branch
2 times, most recently
from
4c1101c to
f8e37a7
Compare
| } FizzyExternalGlobal; | ||
|
|
||
| /// External kind. | ||
| typedef enum FizzyExternalKind |
There was a problem hiding this comment.
Please compare with https://github.com/WebAssembly/wasm-c-api/blob/master/include/wasm.h#L254-L260.
There was a problem hiding this comment.
What do you mean? They're quite similar except that I use full word for "function" and list them in the order of the spec.
There was a problem hiding this comment.
They use uint8_t storage type. So it could be used in C++ directly. Or wrap it in C++ for better control.
typedef uint8_t wasm_externkind_t;
enum wasm_externkind_enum {
WASM_EXTERN_FUNC,
WASM_EXTERN_GLOBAL,
WASM_EXTERN_TABLE,
WASM_EXTERN_MEMORY,
};
// C++
enum class ExternalKind : wasm_externkind_t {
func = WASM_EXTERN_FUNC,
global = WASM_EXTERN_GLOBAL,
table = WASM_EXTERN_TABLE,
memory = WASM_EXTERN_MEMORY,
};This may be done other day, but I'm suggesting exploring the design where some declarations are imported from C to C++ instead of only declaring C equivalents of C++ ones.
There was a problem hiding this comment.
Hmm, this could have been done for FizzyValueType, too 🤔
There was a problem hiding this comment.
True. We may go old-school in this PR and then consider conversion. This probably requires off-site discussion.
There was a problem hiding this comment.
I use full word for "function"
I like this better.
list them in the order of the spec.
So is the wasm-c-api not in order with the spec?
There was a problem hiding this comment.
list them in the order of the spec.
So is the wasm-c-api not in order with the spec?
Yeah, there it's func-global-table-memory.
| /// | ||
| /// @note Only one member of FizzyImportDescription::desc union corresponding to | ||
| /// FizzyImportDescription::kind is defined for each import. | ||
| typedef struct FizzyImportDescription |
There was a problem hiding this comment.
Do we need to mention the lifetime of module/name here? It is explained in fizzy_get_import_description.
Perhaps what we could note here is that the user should refer to the documentation from the function returning this value about lifetimes:
| typedef struct FizzyImportDescription | |
| //// For the lifetime of the #module and #kind fields, please refer to the description provided by the function used to obtain this structure. | |
| typedef struct FizzyImportDescription |
Is #module the correct way?
There was a problem hiding this comment.
Yes, #module and #name worked, but I needed to add individual documentation to the members.
| fizzy_free_module(module); | ||
| } | ||
|
|
||
| TEST(capi, import_name_after_instantiate) |
There was a problem hiding this comment.
Is this testing the lifetime of the name/namespace pointers?
There was a problem hiding this comment.
Yes, checks that it's alive after instantiation, as the documentation claims.
| { | ||
| /* wat2wasm | ||
| (func (import "m" "f1") (result i32)) | ||
| (func (import "m" "f2") (param i64)) |
There was a problem hiding this comment.
Would it make any difference in coverage having a case with multiple params and a result at the same time? Don't want to overcomplicate tests either.
There was a problem hiding this comment.
I'll add it for completeness.
axic
left a comment
axic
left a comment
There was a problem hiding this comment.
Looks good in general, but please squash the renaming commit. Added some smaller remarks.
| /// FizzyImportDescription::kind is defined for each import. | ||
| typedef struct FizzyImportDescription | ||
| { | ||
| const char* module; |
gumb0
force-pushed
the
capi-import-inspection
branch
2 times, most recently
from
993c5a7 to
b4aa0e9
Compare
| c_import_description.desc.memory_limits = wrap(import.desc.memory.limits); | ||
| break; | ||
| case FizzyExternalKindGlobal: | ||
| c_import_description.desc.global_type = wrap(import.desc.global); |
There was a problem hiding this comment.
Hmm, shouldn't this be wrap(import.desc.global.type)?
Never mind I see it includes both type and mutability.
gumb0
force-pushed
the
capi-import-inspection
branch
from
b4aa0e9 to
87a195d
Compare
3 participants
Split from #675