-
Notifications
You must be signed in to change notification settings - Fork 103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new method as_any() to GuestAddressSpace #192
Open
jiangliu
wants to merge
1
commit into
rust-vmm:main
Choose a base branch
from
jiangliu:asany.patch
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, super sorry for taking so long to look at this. The main question in my mind why does this method need to be part of the trait, and not composed or super-imposed somehow externally? Asking because the functionality is quite orthogonal to this trait (and does come up with some extra constraints; not sure how important, but they are there). Is this going to become a pattern that will repeat for other traits we expose in the future?
There are a bunch of ways to get similar functionality without embedding it in the trait proper when downcasting is needed locally in a project (one option/example is illustrated in this test from the
event-manager
crate). Another option is to define a separate trait just for theAny
related part, and include it in a trait bound where required. How do these fit over the use cases you have in mind so far?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All existing code takes an <AS: GuestMemoryAddress>, so we have two ways to extend existing code:
<AS: GuestMemoryAddress>
to<AS: GuestMemoryAddress + AsAny>
GuestMemoryAddress
to provideas_any()
.So it's a tradeoff to reduce code changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry spent a bit more time experimenting with a few alternative solutions, but ultimately it turns out the simplest non-invasive (w.r.t. to the upstream interfaces) are still the aforementioned ones. So to describe the alternative in a bit more detail, I presume the mention of existing code having
<AS: GuestMemoryAddress>
refers to various local implementations. A more straightforward approach to altering each and every occurence of<AS: GuestAddressSpace>
to<AS: GuestAddressSpace + AsAny>
is to define a local trait also namedGuestAddressSpace
(to keep even simple renamings to a minimum) asGuestAddressSpace = upstream::GuestAddressSpace + AsAny
. Then, byuse
-ing the local variant instead of the upstream one (which still requires some changes, but at a hopefully significantly reduced scale), all the existing method/impl definitions should work in a transparent manner (there are some caveats if trait objects are required, but from what we discussed so far that doesn't seem to be a requirement). Is that something that's feasible to do as opposed to the kind of alteration you mentioned?Adding the method to the upstream trait is also something that can be done, but it really feels like it doesn't actually belong there and that it solves a completely orthogonal problem. Also, if we were to go that path, I'm worried that it might become tempting to do this more easily to other traits as well (particularly since this generates additional constraints and alters the semantics at least to some extent).
Ultimately, if the cost is really high for the alternative we can implement the changes for
GuestAddressSpace
, but I'm happy to help with a quick PoC to get the alternative rolling and/or showcase it's actually a working solution (if you can point me at a branch somewhere to add the changes on top of). I really thing it's better overall to avoid conflating the functionality upstream.