Stream: git-cranelift

Topic: cranelift / PR #1352 Use `is_wasm_parameter` in translati...

GitHub (Jan 16 2020 at 19:04):

alexcrichton requested sunfishcode for a review on PR #1352.

GitHub (Jan 16 2020 at 19:04):

alexcrichton opened PR #1352 from filter-more to master:

Added in #1329 it's now possible for multiple parameters to be non-wasm
parameters, so the previous param_types method is no longer suitable
for acquiring all wasm-related parameters, rather then FuncEnvironment
must be consulted. This removes usage of param_types() as a method
from the wasm translation and instead adds a custom method inline for
filtering the parameters based on is_wasm_parameter.

GitHub (Jan 16 2020 at 19:45):

alexcrichton updated PR #1352 from filter-more to master:

Added in #1329 it's now possible for multiple parameters to be non-wasm
parameters, so the previous param_types method is no longer suitable
for acquiring all wasm-related parameters, rather then FuncEnvironment
must be consulted. This removes usage of param_types() as a method
from the wasm translation and instead adds a custom method inline for
filtering the parameters based on is_wasm_parameter.

GitHub (Jan 16 2020 at 19:47):

alexcrichton updated PR #1352 from filter-more to master:

Added in #1329 it's now possible for multiple parameters to be non-wasm
parameters, so the previous param_types method is no longer suitable
for acquiring all wasm-related parameters, rather then FuncEnvironment
must be consulted. This removes usage of param_types() as a method
from the wasm translation and instead adds a custom method inline for
filtering the parameters based on is_wasm_parameter.

GitHub (Jan 16 2020 at 20:09):

sunfishcode submitted PR Review.

GitHub (Jan 16 2020 at 20:09):

sunfishcode created PR Review Comment:

Can you comment on why this is mut and Mut? The ultimate function should be a non-mutating predicate.

GitHub (Jan 16 2020 at 20:36):

alexcrichton updated PR #1352 from filter-more to master:

Added in #1329 it's now possible for multiple parameters to be non-wasm
parameters, so the previous param_types method is no longer suitable
for acquiring all wasm-related parameters, rather then FuncEnvironment
must be consulted. This removes usage of param_types() as a method
from the wasm translation and instead adds a custom method inline for
filtering the parameters based on is_wasm_parameter.

GitHub (Jan 16 2020 at 20:36):

alexcrichton submitted PR Review.

GitHub (Jan 16 2020 at 20:36):

alexcrichton created PR Review Comment:

Oh this is mostly just a Rust idioms thing where you idiomatically take FnMut since it's the most flexible. For brevity and simplicity though I've switched this to Fn, since FnMut wasn't actually needed.

GitHub (Jan 16 2020 at 20:37):

alexcrichton updated PR #1352 from filter-more to master:

Added in #1329 it's now possible for multiple parameters to be non-wasm
parameters, so the previous param_types method is no longer suitable
for acquiring all wasm-related parameters, rather then FuncEnvironment
must be consulted. This removes usage of param_types() as a method
from the wasm translation and instead adds a custom method inline for
filtering the parameters based on is_wasm_parameter.

GitHub (Jan 16 2020 at 20:49):

abrown submitted PR Review.

GitHub (Jan 16 2020 at 20:49):

abrown created PR Review Comment:

It's is_wasm_parameter because it isn't counting the number of ArgumentPurpose::Normal parameters.

@sunfishcode, so the above is a default implementation that embedders are supposed to override?

GitHub (Jan 16 2020 at 21:00):

sunfishcode submitted PR Review.

GitHub (Jan 16 2020 at 21:00):

sunfishcode created PR Review Comment:

Yeah, embedders can override it if they wish. We added this hook because Wasmtime specifically wishes to override it.

GitHub (Jan 17 2020 at 20:09):

alexcrichton requested sunfishcode for a review on PR #1352.

GitHub (Jan 17 2020 at 20:11):

sunfishcode submitted PR Review.

GitHub (Jan 17 2020 at 20:11):

sunfishcode merged PR #1352.

Last updated: Nov 22 2024 at 16:03 UTC