Stream: git-cranelift

Topic: cranelift / Issue #1352 Use `is_wasm_parameter` in transl...

GitHub (Jan 16 2020 at 19:16):

abrown commented on Issue #1352:

I don't understand a couple of things:

• from #1329, why isn't is_wasm_parameter called is_normal_parameter?
• why add a new method instead of just changing param_types and return_types to use this is_wasm_parameter/is_normal_parameter?

GitHub (Jan 16 2020 at 19:17):

abrown edited a comment on Issue #1352:

I don't understand a couple of things:

• from #1329, why isn't is_wasm_parameter called is_normal_parameter? (edit: I mean, don't translations from other languages--e.g. rust--need the same type of check?)
• why add a new method instead of just changing param_types and return_types to use this is_wasm_parameter/is_normal_parameter?

GitHub (Jan 16 2020 at 19:22):

sunfishcode commented on Issue #1352:

It's is_wasm_parameter because it isn't counting the number of ArgumentPurpose::Normal parameters. Wasmtime is now passing extra arguments which are Normal but are not wasm parameters, so we added is_wasm_parameter to the FuncEnvironment trait to let it tell cranelift-wasm which parameters need to be translated as wasm locals.

I think we can just remove the param_types function altogether now. We could remove return_types too, if we added a similar is_wasm_return to FuncEnvironment, which we don't need right now, but would keep things consistent.

GitHub (Jan 16 2020 at 19:45):

alexcrichton commented on Issue #1352:

Sure thing, updated!

GitHub (Jan 17 2020 at 21:43):

alexcrichton commented on Issue #1352:

Would it be ok to get a release to land https://github.com/bytecodealliance/wasmtime/pull/789?

Last updated: Nov 22 2024 at 17:03 UTC