Refactor how InitContext works with factors
#3134
Conversation
alexcrichton
force-pushed
the
refactor-for-has-data
branch
from
005adbf to
ec308bb
Compare
| get_data_with_table, | ||
| } | ||
| } | ||
| pub trait InitContext<F: Factor> { |
There was a problem hiding this comment.
I previously avoided F: Factor since F: Fn is so common, but I was always on the fence about it. 🤷
| self.get_data | ||
| /// Get the instance state for this factor from the store's state. | ||
| fn get_data(store: &mut Self::StoreData) -> &mut FactorInstanceState<F> { | ||
| Self::get_data_with_table(store).0 |
There was a problem hiding this comment.
Oh yeah I guess that makes sense 🙂
crates/factors/src/factor.rs
Outdated
| fn init<C>(&mut self, ctx: &mut C) -> anyhow::Result<()> | ||
| where | ||
| C: InitContext<Self>, |
There was a problem hiding this comment.
Could this be an impl Trait?
| fn init<C>(&mut self, ctx: &mut C) -> anyhow::Result<()> | |
| where | |
| C: InitContext<Self>, | |
| fn init(&mut self, ctx: &mut impl InitContext<Self>) -> anyhow::Result<()> |
There was a problem hiding this comment.
I originally wanted to do this, but it causes issues here for example where the type parameter is being used to monomorphize a new function pointer. There's nothing stopping me though from taking that init and moving it to its own function which has a type parameter and otherwise using impl InitContext everywhere else, however. I wasn't sure how often it'd be necessary to get C for something, and so far it's just WASI but even some of the WASI bits are already in separate functions which can use a parameter instead of an impl Trait.
Would you prefer the impl Trait form for keeping the factor trait relatively clean? I'm kind of on the fence myself but happy to implement either method.
There was a problem hiding this comment.
Gotcha. Yeah, just seemed like it would be less noisy to read but I don't feel strongly about it here.
You seem to be on team "always where clause" which is understandable but we could also fn init<C: InitContext<Self>>. 🤷
Anyway we can merge whenever you'd like.
There was a problem hiding this comment.
Oh personally I don't have much of a preference. When using impl Trait vs T: Trait in a trait itself the Rust compiler ensures that downstream impls all use the same style. For example if the Factor trait used &mut impl InitContext<Self> then all downstream impls must also do so, they can't do C: InitContext<Self>. For inline-bounds vs where clauses, however, there's no such restriction. The trait can use C: InitContext<Self> and impls can use where C: InitContext<Self>. I'm definitely stylistically on the side of preferring where clauses, but that's purely a stylistic preference and I'm happy to change.
I wanted to see what this felt like though so I went ahead and implemented the change with &mut impl InitContext<Self>. I swtiched up the style for how WASI bindings works and I'm liking how it turned out. Mind taking a second look? (no more type parameters or where clauses mostly)
alexcrichton
force-pushed
the
refactor-for-has-data
branch
from
cbbc2db to
307da24
Compare
|
I'll take a look tomorrow. Good to merge? |
|
Indeed yeah should be good to go |
|
Looks like 307da24 isn't signed. |
This commit is a refactoring to replace the concrete `InitContext` structure with a generic type parameter instead. The motivation for this is to prepare for handling bytecodealliance/wasmtime#10770. That's a big change to how `add_to_linker` functions work, notably that the argument to the generated functions is a `fn`, not a `F: Fn`. WASI factors currently rely on the closure-like nature this argument which means it's not compatible with that change. The refactoring to use a `trait InitContext` here enables plumbing a type parameter through a function to be able to get a function pointer without relying on closures. Signed-off-by: Alex Crichton <[email protected]>
Signed-off-by: Alex Crichton <[email protected]>
alexcrichton
force-pushed
the
refactor-for-has-data
branch
from
872b5f6 to
11ed60a
Compare
|
One day I'll set up auto-signing... |
|
If y'all are waiting on me to merge, mind merging for me? (I don't have The Button) If you're waiting to get a chance to review again, no worries! |
This commit is a refactoring to replace the concrete
InitContextstructure with a generic type parameter instead. The motivation for this is to prepare for handling bytecodealliance/wasmtime#10770. That's a big change to howadd_to_linkerfunctions work, notably that the argument to the generated functions is afn, not aF: Fn. WASI factors currently rely on the closure-like nature this argument which means it's not compatible with that change. The refactoring to use atrait InitContexthere enables plumbing a type parameter through a function to be able to get a function pointer without relying on closures.