Skip to content

Workaround for wasm2js output minification issue with emscripten#2185

Merged
kripken merged 2 commits intoWebAssembly:masterfrom
bvibber:wasm2js-min
Jul 1, 2019
Merged

Workaround for wasm2js output minification issue with emscripten#2185
kripken merged 2 commits intoWebAssembly:masterfrom
bvibber:wasm2js-min

Conversation

@bvibber
Copy link
Copy Markdown
Contributor

@bvibber bvibber commented Jun 28, 2019

When using emscripten with -O2 and --memory-init-file 0, the
JS minification breaks on this function for memory initialization
setup, causing an exception to be thrown during module setup.

Moving from two 'var' declarations for the same variable to one
should avoid hitting this with no change in functionality (the
var gets hoisted anyway).

emscripten-core/emscripten#8886

When using emscripten with -O2 and --memory-init-file 0, the
JS minification breaks on this function for memory initialization
setup, causing an exception to be thrown during module setup.

Moving from two 'var' declarations for the same variable to one
should avoid hitting this with no change in functionality (the
var gets hoisted anyway).

emscripten-core/emscripten#8886
@tlively
Copy link
Copy Markdown
Member

tlively commented Jun 28, 2019

Looks good to me! What a strange bug. We hope to get the CI unbroken soon so this can get in.

@tlively
Copy link
Copy Markdown
Member

tlively commented Jun 29, 2019

@Brion It looks like the test failures are legit. Can you update the tests?

Manually hoisted a var declaration to work around a minifier bug
when used with emscripten at -O2 and above.
@bvibber
Copy link
Copy Markdown
Contributor Author

bvibber commented Jun 29, 2019

@Brion It looks like the test failures are legit. Can you update the tests?

Done. Hopefully all clear this time! :D

@kripken
Copy link
Copy Markdown
Member

kripken commented Jul 1, 2019

Looks good, thanks @Brion!

@kripken kripken merged commit ab34a77 into WebAssembly:master Jul 1, 2019
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