Stream: git-wasmtime

Topic: wasmtime / PR #5316 Fix issues in `FunctionBuilder` API

Wasmtime GitHub notifications bot (Nov 22 2022 at 17:06):

Rodrigodd opened PR #5316 from fix-FunctionBuilder-API to main:

<!--

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

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

• [x] 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.

• [x] 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.
  -->
  Here on this zulip stream, I have asked about some inconsistencies in the FunctionBuilderContext and FunctionBuilder APIs, and it was suggested that I could probably fix these issues. This PR addresses two of them.

FunctionBuilder is supposed to be used to only build a single Function, but FunctionBuilder::finalize() indicates that it can be used for multiples Functions. The first commit fixed that by making finalize() consume self. Nowhere in the codebase FunctionBuilder was reused, so the change was very straight forward.

The other one was that FunctionBuilder::func was public, which could allow the Function to be messed up mid-translation. I tried replacing it by an immutable func() getter, but in the codebase func is in fact messed with mid-translation. So, I also added a func_mut() getter. I am not sure if this change brings meaningful benefits, so I could remove it if you prefer.

Wasmtime GitHub notifications bot (Nov 22 2022 at 17:18):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Nov 22 2022 at 17:27):

bjorn3 created PR review comment:

cg_clif uses fx.builder.finalize() in a function taking fx by mutable reference. As such moving out is not allowed. It should be possible to change this though.

Wasmtime GitHub notifications bot (Nov 22 2022 at 17:27):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 22 2022 at 18:44):

Rodrigodd updated PR #5316 from fix-FunctionBuilder-API to main.

Wasmtime GitHub notifications bot (Nov 23 2022 at 12:05):

Rodrigodd updated PR #5316 from fix-FunctionBuilder-API to main.

Wasmtime GitHub notifications bot (Nov 23 2022 at 12:43):

Rodrigodd requested bjorn3 for a review on PR #5316.

Wasmtime GitHub notifications bot (Nov 23 2022 at 12:43):

Rodrigodd requested jameysharp for a review on PR #5316.

Wasmtime GitHub notifications bot (Nov 23 2022 at 12:43):

Rodrigodd requested bjorn3 for a review on PR #5316.

Wasmtime GitHub notifications bot (Nov 23 2022 at 12:43):

Rodrigodd requested jameysharp for a review on PR #5316.

Wasmtime GitHub notifications bot (Nov 23 2022 at 12:43):

Rodrigodd requested bjorn3 for a review on PR #5316.

Wasmtime GitHub notifications bot (Nov 23 2022 at 12:44):

Rodrigodd requested jameysharp for a review on PR #5316.

Wasmtime GitHub notifications bot (Nov 23 2022 at 12:49):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 23 2022 at 23:05):

Rodrigodd updated PR #5316 from fix-FunctionBuilder-API to main.

Wasmtime GitHub notifications bot (Nov 23 2022 at 23:12):

jameysharp has enabled auto merge for PR #5316.

Wasmtime GitHub notifications bot (Nov 23 2022 at 23:41):

jameysharp merged PR #5316.

Last updated: Dec 23 2024 at 13:07 UTC