Skip to content
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

add Binaryen(Get|Set)AllowHeavyweight to binaryen-c.h #3082

Merged
merged 6 commits into from
Aug 28, 2020

Conversation

MaxGraey
Copy link
Contributor

src/binaryen-c.h Outdated Show resolved Hide resolved
src/binaryen-c.h Outdated Show resolved Hide resolved
MaxGraey and others added 2 commits August 28, 2020 15:10
Co-authored-by: Daniel Wirtz <dcode@dcode.io>
Co-authored-by: Daniel Wirtz <dcode@dcode.io>
@MaxGraey MaxGraey requested a review from dcodeIO August 28, 2020 12:11
@dcodeIO
Copy link
Contributor

dcodeIO commented Aug 28, 2020

Looks good, this will fix the issue encountered on our end. Wondering though if we should add a test calling allowHeavyweight and the other (rather new) optimization options like inlining sizes to test/binaryen.js/kitchen-sink.js to test/binaryen.js/inlining-options.js, to be sure.

@dcodeIO dcodeIO merged commit fecfa92 into WebAssembly:master Aug 28, 2020
@MaxGraey MaxGraey deleted the add-allowheavyweight-to-header branch August 28, 2020 13:25
@dcodeIO
Copy link
Contributor

dcodeIO commented Aug 28, 2020

Took the liberty to merge this one as it's rather straight forward and only affecting parts of Binaryen that I'm relatively familiar with. Let me know if you'd prefer to be involved in review of trivial fixes like these instead :)

@kripken
Copy link
Member

kripken commented Aug 31, 2020

@dcodeIO 👍 I think it's great to review and merge straightforward stuff like this as you did!

And thanks @MaxGraey for the PR!

@MaxGraey
Copy link
Contributor Author

@kripken Thanks you! Btw I also found sometimes we could got compiler's halt problem when try inline recurcive functions, so I created another PR which fix that: #3085

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants