-
Notifications
You must be signed in to change notification settings - Fork 129
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
chore(wasmer): add helpers.go file with helper functions #2749
Conversation
011acc6
to
4316030
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## development #2749 +/- ##
===============================================
+ Coverage 63.02% 63.25% +0.23%
===============================================
Files 213 213
Lines 26965 26957 -8
===============================================
+ Hits 16994 17051 +57
+ Misses 8425 8358 -67
- Partials 1546 1548 +2 |
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.
LGTM, just a suggestion: since all the operations relies on the wasmer.InstanceContext
could we rename the file from helpers.go
to context.go
or instance_context.go
?
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.
lgtm
51ec274
to
170d651
Compare
Good suggestion, although |
869fd3c
to
38ae18f
Compare
Looks like Deepsource doesn't like having two cgo files in the same package and fails compiling, I'll convert this back to draft and fix this later. Maybe split this in another subpackage for the sake of deepsource 🤔 |
FYI, I noticed this was in prep of a possible use of wazero, and thanks for thinking of us. wazero just cut its first beta tag (v1.0.0-beta.1) and also opened a gophers slack We've also been working harder on our website for things people usually can't find https://wazero.io/languages/ Wish you well and hope to see you around. |
Awesome @codefromthecrypt thanks for letting us know! We're probably a few months away from switching to wazero due to other priorities, but it's definitely something we would all like to jump to for sure! |
b117e8d
to
242b98e
Compare
- Use `ptr` instead of `out` for 32 bit pointers - Use `pointerSize` instead of `span` for 64 bit pointers size - Name return values - Other minor renamings such as `res` to `result`, `enc` to `encodedResult`
- Use slice capacity allocation - Use `copy` - Remove unneeded appends
242b98e
to
040c139
Compare
- Split out (CGO related) helper functions from `imports.go` to `lib/runtime/wasmer/helpers.go` - Move pointer size helper functions to `lib/runtime/wasmer/helpers.go` - Change `toWasmMemorySized` to NOT take a size argument (unneeded) - Clarify all comments for helper functions - Update all error wrappings - Review variable names - Use `ptr` instead of `out` for 32 bit pointers - Use `pointerSize` instead of `span` for 64 bit pointers size - Name return values - Other minor renamings such as `res` to `result`, `enc` to `encodedResult` - Optimizations: - `storageAppend`: use slice capacity allocation, `copy` and remove unneeded `append`s - `toKillStorageResultEnum`: use `copy` instead of `append` - `toWasmMemoryOptional`: remove unneeded variable copy
Changes
imports.go
tolib/runtime/wasmer/helpers.go
lib/runtime/wasmer/helpers.go
toWasmMemorySized
to NOT take a size argument (unneeded)ptr
instead ofout
for 32 bit pointerspointerSize
instead ofspan
for 64 bit pointers sizeres
toresult
,enc
toencodedResult
storageAppend
: use slice capacity allocation,copy
and remove unneededappend
stoKillStorageResultEnum
: usecopy
instead ofappend
toWasmMemoryOptional
: remove unneeded variable copyTests
go test -tags integration github.com/ChainSafe/gossamer
Issues
Friday evening fun.
Also aiming to make the migration to wazero easier.
Primary Reviewer
@EclesioMeloJunior