go-interpreter / wagon

wagon, a WebAssembly-based Go interpreter, for Go.

BSD 3-Clause "New" or "Revised" License

902 stars 150 forks source link

LocalNames and NameMap's Marshal Function Bug Fix #187

Closed zhangzhiqiangcs closed 4 years ago

zhangzhiqiangcs commented 4 years ago

I use NameSection in my project. I think NameMap and LocalNames' Marshal function miss out size information constrasted to it's Unmarshal function.

sbinet commented 4 years ago

Codecov Report

Merging #187 into master will decrease coverage by 0.05%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
- Coverage   69.77%   69.72%   -0.06%     
==========================================
  Files          49       49              
  Lines        5495     5499       +4     
==========================================
  Hits         3834     3834              
- Misses       1318     1322       +4     
  Partials      343      343

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 8dd99d5...614874e. Read the comment docs.

codecov-io commented 4 years ago

Codecov Report

Merging #187 into master will increase coverage by 0.47%. The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
+ Coverage   69.77%   70.24%   +0.47%     
==========================================
  Files          49       49              
  Lines        5495     5499       +4     
==========================================
+ Hits         3834     3863      +29     
+ Misses       1318     1283      -35     
- Partials      343      353      +10

Impacted Files	Coverage Δ
wasm/section.go	`58.93% <0%> (+4.06%)`	: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 8dd99d5...19c7109. Read the comment docs.

sbinet commented 4 years ago

thanks. could you add a few tests so we don't introduce regressions unadvertantly?

thanks again.

zhangzhiqiangcs commented 4 years ago

thanks. could you add a few tests so we don't introduce regressions unadvertantly?

thanks again.

No problem, I'll do it later.

twitchyliquid64 commented 4 years ago

Thanks for writing this! I've double-checked the spec, this seems correct. No idea how this worked in the first place!

zhangzhiqiangcs commented 4 years ago

Thanks for writing this! I've double-checked the spec, this seems correct. No idea how this worked in the first place!

In fact, the module's encode and decode function don't Marshal and Unmarshal NameSubSection's data. It matters when you modified the NameSubSection and want to write back to the Module.

zhangzhiqiangcs commented 4 years ago

just a few nitpicks. to address.

could you also send a PR against go-interpreter/license, adding yourself to the AUTHORS and CONTRIBUTORS files?

I have done this in the pr https://github.com/go-interpreter/license/pull/19. Thanks a lot~