Open sampaioletti opened 5 years ago
How are you constructing the Module
?
We should think about how the API for manually building a module should look like.
Some ideas:
ModuleBuilder{}
with methods to add globals/functions/etc, and then you call Build()
.Build()
or Setup()
method to type Module
, so the user can get these things populated.ResolveModule
package method for bringing in user-specified modules.What do you think?
I'm constructing the module in the vain of that found in the Example from the docs
Either way I think we could at least come up with some helper functions that make it easier to develop user modules (happy to take a stab once this part is discussed)
If opt 1 was chosen..then maybe either some documentation around the Module struct type or evaluating if some fields can be made private.
If 2 then I have a branch where I was pondering the same thing...something along the lines of...
type BuilderOpts = func(*Module)
func ModuleBuilder(opts ...BuilderOpts) *Module {
m := NewModule()
for _, v := range opts {
//optionally have a return type with error
v(m)
}
//add finilization
return m
}
//or
type Builder struct {
m *Module
}
func (b *Builder) WithFunc() {
//add func to module
}
func (b *Builder) Build() *Module {
//add finilization
return b.m
}
I dont know that I have an opinion..nice thing about the first is with no ops it just makes a module..so could technically just modify the NewModule function...i've seen a lot of go libs doing that...so I think the first one might be more idiomatic...but I'm used to the second option personally. Your call.
I think the 'builder' pattern makes the most sense for the use case where developers are dynamically building modules.
That said, I don't have a strong opinion here so feel free to lead this in a different direction if you do.
Merging #164 into master will increase coverage by
<.01%
. The diff coverage is75%
.
@@ Coverage Diff @@
## master #164 +/- ##
==========================================
+ Coverage 69.71% 69.71% +<.01%
==========================================
Files 48 48
Lines 5484 5492 +8
==========================================
+ Hits 3823 3829 +6
- Misses 1317 1318 +1
- Partials 344 345 +1
Impacted Files | Coverage Δ | |
---|---|---|
wasm/imports.go | 43.68% <75%> (+2.63%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 01d3d33...83180e4. Read the comment docs.
My only concern is that the populate* methods append to the internal slices, so if they are called twice (as they would be because they are called in ReadModule
and resolveImports
in this PR), there will be duplicate entries.
Does it work if you zero out GlobalIndexSpace / FunctionIndexSpace etc etc, before you run the populate methods? That way we can avoid the duplicate problem.
While playing with my emlibc branch I was having to explicitly set my module.GlobalIndexSpace for my imports rather than just specifying module.Global after comparing the code in wasm/imports.go to wasm/module.go it appears we may be missing the initiliization code
So I dropped it in after a module is imported and it seemed to work. I'm nor sure if this has other implications or if the original is the intended behavior...so i'm sharing
Thanks.