Stream: git-wasmtime

Topic: wasmtime / PR #6443 winch: Drop `FuncEnv` trait

Wasmtime GitHub notifications bot (May 24 2023 at 13:38):

saulecabrera requested alexcrichton for a review on PR #6443.

Wasmtime GitHub notifications bot (May 24 2023 at 13:38):

saulecabrera opened PR #6443 from saulecabrera:winch-drop-func-env to bytecodealliance:main:

This commit is a small cleanup to drop the usage of the FuncEnv trait.

In https://github.com/bytecodealliance/wasmtime/pull/6358, we agreed on making winch-codegen directly depend on wasmtime-environ.

Introducing a direct relatioship between winch-codegen and wasmtime-environ means that the FuncEnv trait is no longer serving its original purpose, and we can drop the usage of the trait and use the types exposed from winch-codegen directly instead.

Even though this change drops the FuncEnv trait, it still keeps a FuncEnv struct, which is used during code generation.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (May 24 2023 at 13:38):

saulecabrera requested elliottt for a review on PR #6443.

Wasmtime GitHub notifications bot (May 24 2023 at 13:38):

saulecabrera requested wasmtime-compiler-reviewers for a review on PR #6443.

Wasmtime GitHub notifications bot (May 24 2023 at 13:38):

saulecabrera requested wasmtime-core-reviewers for a review on PR #6443.

Wasmtime GitHub notifications bot (May 24 2023 at 13:38):

saulecabrera requested wasmtime-default-reviewers for a review on PR #6443.

Wasmtime GitHub notifications bot (May 24 2023 at 13:52):

saulecabrera updated PR #6443.

Wasmtime GitHub notifications bot (May 24 2023 at 14:54):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 24 2023 at 14:54):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 24 2023 at 14:54):

alexcrichton created PR review comment:

I'm not all that familiar with the design of these generics, but I would naively expect that a choice of either MacroAssembler or ABI would imply PtrSize. If that's the case would it perhaps be possible to have P be an associated type of either prior trait?

Or would it, for example, make sense to put both ABI and PtrSize inside of the MacroAssembler trait as an associated type?

Wasmtime GitHub notifications bot (May 24 2023 at 15:12):

saulecabrera created PR review comment:

That makes sense. Taking a closer look, I'm leaning towards having the PtrSize as an associated type inside the ABI trait, which already provides similar information (e.g. word_bytes).

Wasmtime GitHub notifications bot (May 24 2023 at 16:35):

saulecabrera created PR review comment:

Upon a second look, having both of these as associated types in the MacroAssembler makes more sense I think, and it's going to result in quite a decent cut in boilerplate. I started working on the change, but I'm anticipating that it's going to be a decent amount of changes unrelated to this PR, so I think I prefer landing this and putting the refactor once this lands.

Wasmtime GitHub notifications bot (May 24 2023 at 16:37):

saulecabrera created PR review comment:

It's also in my todo to standardize the signatures in the ABI trait (e.g. there's no reason to have them borrow &self), so I'm thinking I'll bundle those improvements in the refactoring too.

Wasmtime GitHub notifications bot (May 24 2023 at 17:08):

saulecabrera updated PR #6443.

Wasmtime GitHub notifications bot (May 24 2023 at 18:16):

saulecabrera merged PR #6443.

Last updated: Nov 22 2024 at 17:03 UTC