Add new method as_any() to GuestAddressSpace #192
Conversation
jiangliu
force-pushed
the
asany.patch
branch
3 times, most recently
from
b917fbe
to
a3cd5d3
Compare
|
|
||
| fn memory(&self) -> Self::T { | ||
| GuestMemoryLoadGuard { guard: self.load() } | ||
| } | ||
|
|
||
| fn as_any(&self) -> &dyn Any { |
There was a problem hiding this comment.
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 the Any 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.
All existing code takes an <AS: GuestMemoryAddress>, so we have two ways to extend existing code:
- Change all instances of
<AS: GuestMemoryAddress>to<AS: GuestMemoryAddress + AsAny> - Enhance
GuestMemoryAddressto provideas_any().
So it's a tradeoff to reduce code changes.
There was a problem hiding this comment.
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 named GuestAddressSpace (to keep even simple renamings to a minimum) as GuestAddressSpace = upstream::GuestAddressSpace + AsAny. Then, by use-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.
|
@jiangliu can you fix the conflicts so we can merge the PR? Thanks. |
Add new method as_any() to GuestAddressSpace, which returns reference to a std::any::Any trait object. Then downcast_ref() could be used to get the concrete type of GuestAddressSpace trait object. Signed-off-by: Liu Jiang <[email protected]>
|
(Did not mean to close, sorry). |
Add new method as_any() to GuestAddressSpace, which returns reference
to a std::any::Any trait object. Then downcast_ref() could be used to
get the concrete type of GuestAddressSpace trait object.
Signed-off-by: Liu Jiang [email protected]
Summary of the PR
Please summarize here why the changes in this PR are needed.
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commitmessage has max 60 characters for the summary and max 75 characters for each
description line.
test.
unsafecode is properly documented.