elliottt opened PR #5450 from trevor/public-insts-field
to main
:
We have some operations defined on DataFlowGraph purely to work around borrow-checker issues with InstructionData and other data on DataFlowGraph. Part of the problem is that indexing the DFG directly hides the fact that we're only indexing the
insts
field of the DFG.This PR makes the
insts
field of the DFG public, but wraps it in a newtype that only allows indexing. This means that the borrow checker is better able to tell when oeprations on memory held by the DFG won't conflict, which comes up frequently when mutating ValueLists held by InstructionData.<!--
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.
-->
jameysharp submitted PR review.
jameysharp created PR review comment:
Pre-existing: I think this function leaks the portion of the pool corresponding to
old_actuals
. Doesn't need to be fixed in this PR, I just wanted to note it.
jameysharp created PR review comment:
I keep being confused about what promises we make about serialization. I assume inserting a newtype wrapper changes the derived serialization format. Do we care? I think we don't care because anything we might be serializing is only meant to be stable within the same major version of Wasmtime, but I don't know.
jameysharp created PR review comment:
I suppose this case can use the
IndexMut
implementation on the newtype wrapper, instead of needing to dig inside it. I guess I have a mild preference for using the public API where that's reasonable.
jameysharp submitted PR review.
jameysharp created PR review comment:
This change makes me so much happier :grin:
elliottt submitted PR review.
elliottt created PR review comment:
I was just digging into this, and found that this function handles cases where the arguments aren't contained in an entity list as well.
elliottt created PR review comment:
Should we open an issue?
elliottt submitted PR review.
elliottt updated PR #5450 from trevor/public-insts-field
to main
.
elliottt updated PR #5450 from trevor/public-insts-field
to main
.
elliottt created PR review comment:
elliottt submitted PR review.
elliottt submitted PR review.
elliottt created PR review comment:
I think I had a recency bias issue and thought you were referring to refactoring
arguments_mut
instead of removing the unnecessary use of.0
. I'll agree that this is better without the.0
:+1:
elliottt updated PR #5450 from trevor/public-insts-field
to main
.
elliottt has marked PR #5450 as ready for review.
elliottt requested fitzgen for a review on PR #5450.
elliottt requested cfallin for a review on PR #5450.
elliottt created PR review comment:
@cfallin or @fitzgen, do we make any guarantees about the stability of the serialized format for these structures between releases?
elliottt submitted PR review.
elliottt edited PR #5450 from trevor/public-insts-field
to main
:
We have some operations defined on DataFlowGraph purely to work around borrow-checker issues with InstructionData and other data on DataFlowGraph. Part of the problem is that indexing the DFG directly hides the fact that we're only indexing the
insts
field of the DFG.This PR makes the
insts
field of the DFG public, but wraps it in a newtype that only allows indexing. This means that the borrow checker is better able to tell when operations on memory held by the DFG won't conflict, which comes up frequently when mutating ValueLists held by InstructionData.<!--
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.
-->
cfallin submitted PR review.
cfallin created PR review comment:
Nope, this feeds
cfallin edited PR review comment.
jameysharp submitted PR review.
elliottt merged PR #5450.
Last updated: Dec 23 2024 at 13:07 UTC