refactor: Use span instead of vectors in ExternalFunction #668
Conversation
This file contains 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
Codecov Report
@@ Coverage Diff @@
## master #668 +/- ##
=======================================
Coverage 99.28% 99.28%
=======================================
Files 72 72
Lines 9918 9954 +36
=======================================
+ Hits 9847 9883 +36
Misses 71 71
Flags with carried forward coverage won't be shown. Click here to find out more.
|
gumb0
force-pushed
the
external-function-type-span
branch
9 times, most recently
from
fc6bff3
to
38844d9
Compare
gumb0
force-pushed
the
external-function-type-span
branch
from
38844d9
to
27fb67c
Compare
chfast
approved these changes
Dec 8, 2020
gumb0
force-pushed
the
external-function-type-span
branch
from
27fb67c
to
06c2923
Compare
Added comments to |
axic
reviewed
Dec 29, 2020
| @@ -59,6 +59,8 @@ class span | |||
| constexpr T* data() const noexcept { return m_begin; } | |||
| constexpr std::size_t size() const noexcept { return m_size; } | |||
|
|
|||
| constexpr bool empty() const noexcept { return m_size == 0; } | |||
There was a problem hiding this comment.
Matches std definition: https://en.cppreference.com/w/cpp/container/span/empty
axic
reviewed
Dec 29, 2020
lib/fizzy/instantiate.hpp
Outdated
| @@ -22,9 +22,14 @@ namespace fizzy | |||
| struct ExecutionResult; | |||
| struct Instance; | |||
|
|
|||
| // Function representing WebAssembly or host function execution. | |||
There was a problem hiding this comment.
Can you make these (and on the struct) comments doxygen?
gumb0
force-pushed
the
external-function-type-span
branch
from
06c2923
to
1171838
Compare
axic
approved these changes
Jan 4, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This allows to avoid extra vector allocation when going from C
FizzyExternalFunctionto C++fizzy::ExternalFunctionand also when C++ API returns fromfind_exported_function().Also I it helps with
std::functionrefactoring in #643The downside is that user has to ensure that arrays pointed to by spans live long enough (usually these arrays are in
Instance, and you need thisInstanceto runExternalFunctionanyway).This leaves
ImportedFunctionstill usingvector<ValType>for input types, I think it's convenient to leave it like this, otherwise it wouldn't be possible to pass input types as inline initializer list like this:neither like this
std::vector<ImportedFunction> imported_functions = { {"mod1", "foo1", {}, ValType::i32, function_returning_value(0)}, {"mod1", "foo2", {ValType::i32}, ValType::i32, function_returning_value(1)}, {"mod2", "foo1", {ValType::i32}, ValType::i32, function_returning_value(2)}, {"mod2", "foo2", {ValType::i64, ValType::i32}, std::nullopt, function_returning_void}, };(Spans have to point to some container, so explicit vector declarations would be required on the user side)
Also note that to make above use cases work,
resolve_imported_functions()producesExternalFunctions with typespans pointing to the type vector insideModule, not to the passed vector. Not sure whether it's confusing from the API standpoint, but it works.