Stream: git-wasmtime

Topic: wasmtime / PR #5770 Move the default block to the front o...

Wasmtime GitHub notifications bot (Feb 13 2023 at 19:51):

elliottt opened PR #5770 from trevor/default-block-first to main:

The new api on JumpTableData makese it easy to keep the default label first, and that shrinks the diff in #5731 a bit.
<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Feb 13 2023 at 19:52):

elliottt requested jameysharp for a review on PR #5770.

Wasmtime GitHub notifications bot (Feb 13 2023 at 20:06):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 13 2023 at 20:06):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 13 2023 at 20:06):

jameysharp created PR review comment:

While reviewing this I considered two alternative implementations but decided I like the one you've chosen best.

One alternative is to continue to accept a Vec and use table.insert(0, def), which implicitly does a memmove. Some callers already have a Vec allocated and it's a little annoying to have to allocate a second one to copy into, only for the caller to drop theirs immediately afterward.

The other alternative is to take table: impl IntoIterator<Item = Block>. That's somewhat appealing for cranelift/wasm/src/code_translator.rs which I think could then avoid allocating a temporary Vec. It also works fine for all the tests since arrays implement the right IntoIterator. Passing in a Vec by-value works too, although like the current approach that still allocates a new Vec and copies into it. However, I think this is a little more magic than necessary. Passing in a slice is fine.

Wasmtime GitHub notifications bot (Feb 13 2023 at 20:12):

elliottt has enabled auto merge for PR #5770.

Wasmtime GitHub notifications bot (Feb 13 2023 at 20:50):

elliottt merged PR #5770.

Last updated: Dec 23 2024 at 13:07 UTC