Conversation
kripken
commented
Nov 28, 2017
- Removes unneeded code from the wasm loading glue.
- Ifdefs code not needed in the normal (just wasm-native case).
- Add a comprehensive test for the modes.
- Remove support for changing the binaryen mode at runtime. It hasn't actually worked in a while, and was never tested, and really isn't that important.
…ging the binaryen method at runtime, which we in fact did not fully support for a while now
| asm = open('a.wasm.asm.js').read() | ||
| asm = asm.replace('"almost asm"', '"use asm"; var not_in_asm = [].length + (true || { x: 5 }.x);') | ||
| asm = asm.replace("'almost asm'", '"use asm"; var not_in_asm = [].length + (true || { x: 5 }.x);') | ||
| with open('a.wasm.asm.js', 'w') as o: o.write(asm) |
There was a problem hiding this comment.
Is this going to work on all OSes (i.e. you open the file for reading, and then open it for writing without closing it)?
tests/test_other.py
Outdated
| if not success: | ||
| try_delete('a.wasm.wasm') | ||
| else: | ||
| 1/0 |
There was a problem hiding this comment.
Now that we have future integer division, you can do 1//0 :D
There was a problem hiding this comment.
Ok ok, I wrote a proper throw ;)
| try_delete('a.wasm.wasm') | ||
| else: | ||
| 1/0 | ||
| proc = subprocess.Popen(NODE_JS + ['a.wasm.js'], stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
There was a problem hiding this comment.
Can we start using shared.execute() (or some similar common infrastructure) in new code?
There was a problem hiding this comment.
Generally yes, but here we need the return code so a lower-level API is used.
There was a problem hiding this comment.
ok, can you use it for the check_call above then?
There was a problem hiding this comment.
Actually isn't check_call good to use?
My understanding is we need a wrapper for the universal newlines problem, but that only applies when we want to read the output - if there's no output, the newlines problem isn't there.
There was a problem hiding this comment.
Maybe; but if check_call is perfect, then we should get rid of shared.check_call. Otherwise we should use the latter everywhere; we have a lot of inconsistency right now. At least the logging looks nice. And it looks like execute has some special handling for Windows, do we want that here too?
There was a problem hiding this comment.
Yeah, we should improve this. I think we should use the shared.py versions in the tools themselves, as they log out better errors (which matters less in the tests). But maybe that's not true. And I'm not sure why we have that weird code in execute, also:
cmd[0] = Building.remove_quotes(cmd[0])
How about if I open an issue to discuss this?
There was a problem hiding this comment.
Sure, sounds good. You can go ahead and land this, there will be general cleanup either way.
|
LGTM with the check_call->shared.execute change. |