C API execute with dynamically allocated module#576
Merged
Conversation
Codecov Report
@@ Coverage Diff @@
## master #576 +/- ##
==========================================
+ Coverage 98.23% 98.24% +0.01%
==========================================
Files 63 63
Lines 9011 9180 +169
==========================================
+ Hits 8852 9019 +167
- Misses 159 161 +2 |
gumb0
force-pushed
the
dynamic-module
branch
2 times, most recently
from
af24f83 to
0629668
Compare
gumb0
force-pushed
the
capi-execute-dynamic-module
branch
from
2c89c24 to
f74969b
Compare
gumb0
force-pushed
the
dynamic-module
branch
2 times, most recently
from
82f32cc to
de689af
Compare
gumb0
force-pushed
the
capi-execute-dynamic-module
branch
from
f74969b to
957f3ee
Compare
gumb0
force-pushed
the
dynamic-module
branch
2 times, most recently
from
7aea9d2 to
f5f5c3b
Compare
gumb0
force-pushed
the
capi-execute-dynamic-module
branch
from
957f3ee to
2a97ebe
Compare
Closed
axic
reviewed
Oct 12, 2020
axic
reviewed
Oct 12, 2020
include/fizzy/fizzy.h
Outdated
| bool trapped; | ||
| /// Whether function returned a value. Valid only if trapped == false | ||
| bool has_value; | ||
| /// Value returned from a function. Valid only if has_value == true |
Member
There was a problem hiding this comment.
Suggested change
| /// Value returned from a function. Valid only if has_value == true | |
| /// Value returned from a function. Valid only if has_value equals true. |
Can this be true even if trap is true?
Collaborator
Author
There was a problem hiding this comment.
Clarified in comment
axic
reviewed
Oct 12, 2020
axic
reviewed
Oct 12, 2020
axic
reviewed
Oct 12, 2020
chfast
reviewed
Oct 12, 2020
axic
reviewed
Oct 12, 2020
gumb0
force-pushed
the
capi-execute-dynamic-module
branch
from
ad3da6a to
e683ce3
Compare
axic
reviewed
Oct 12, 2020
| return [func, context](fizzy::Instance& instance, fizzy::span<const fizzy::Value> args, | ||
| int depth) noexcept -> fizzy::ExecutionResult { | ||
| const auto result = func(context, wrap(&instance), wrap(args.data()), args.size(), depth); | ||
| return unwrap(result); |
Member
There was a problem hiding this comment.
This takes result as a reference. Will that reference be valid after exiting this lambda? Is the wrap(result.value) doing a copy?
Collaborator
Author
There was a problem hiding this comment.
Both wrap and unwrap for result are doing a copy
axic
reviewed
Oct 12, 2020
| /// Parse binary module. | ||
| const FizzyModule* fizzy_parse(const uint8_t* wasm_binary, size_t wasm_binary_size); | ||
|
|
||
| /// Free resources associated with the module. |
Member
There was a problem hiding this comment.
Perhaps mention module must be non-null.
Member
There was a problem hiding this comment.
And the same comments apply to instantiate.
Collaborator
Author
There was a problem hiding this comment.
This calles delete, so is guaranteed to be no-op for null pointer. I'll mention this and add a test.
axic
reviewed
Oct 12, 2020
| /// Validate binary module. | ||
| bool fizzy_validate(const uint8_t* wasm_binary, size_t wasm_binary_size); | ||
|
|
||
| /// Parse binary module. |
Member
There was a problem hiding this comment.
Perhaps mention this returns a non-null module pointer on success.
Member
|
Looks good otherwise. |
axic
reviewed
Oct 12, 2020
axic
reviewed
Oct 12, 2020
gumb0
force-pushed
the
capi-execute-dynamic-module
branch
from
4ae9696 to
7a5ec49
Compare
axic
approved these changes
Oct 12, 2020
Member
axic
left a comment
axic
left a comment
There was a problem hiding this comment.
Please squash and add a test for free_module(nullptr) and free_instance(nullptr).
Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
gumb0
force-pushed
the
capi-execute-dynamic-module
branch
from
f38f7e5 to
59419bd
Compare
chfast
approved these changes
Oct 12, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alternative to #533 based on #575