Paolo Bonzini <[email protected]> writes:
> On Tue, Feb 11, 2025 at 7:47 AM Junjie Mao <[email protected]> wrote: >> I would suggest to keep the "sysbus" prefix in the method name, or in >> general, keep the class prefix in the method names in XXXClassMethods >> traits. Otherwise APIs from different parent classes may also >> conflict. As an example, in the following class hierarchy: >> >> Object -> DeviceState -> PCIDevice -> PCIBridge -> ... >> >> For PCIDevice instances there is an API pci_device_reset(), and for >> PCIBridge ones pci_bridge_reset(). Without class prefixes both will be >> wrapped (as blanket impl in two traits) as reset() and a dev.reset() >> call will lead to a "multiple applicable items in scope" build error. > > Yes, reset is definitely a problem. > > I will wrap qdev_realize() in DeviceMethods to check if you get > "multiple applicable items" from that as well. > > I can also go back and add "sysbus_" in front of init_mmio, init_irq, > etc.; but on the other hand we have Timer::{modify, delete} and > DeviceMethods::init_{clock,qdev}_{in,out}, should they be changed as > well? I'm not sure there is a single solution that always works. > The DeviceMethods::init_* wrappers may need a similar prefix for the same reason, though it seems unlikely to suffer from such name conflicts. Meanwhile, adding prefixes to timer::* seems redundant. Essentially we want a naming convention that (1) avoids any potential name conflicts, and (2) applies consistently to (ideally) all APIs. For goal (1), we need something at API call sites that can resolve the potential ambiguity. So instead of dev.realize(), we may write: a) dev.sysbus_realize() b) SysBusDeviceMethods::realize(&dev); dev.realize() is still ok if there is no ambiguity c) dev.as_ref::<SysBusDevice>().realize() (any more options?) None looks perfect, unfortunately. Option (a) introduces inconsistent naming conventions as mentioned earlier; (b) cannot prevent confusions when a device has both a "reset()" method and "dev.reset()" calls; (c) deviates from how wrappers are auto-delegated to child classes today and is the longest to write. Option (b) + a lint that warns same method names looks a good tradeoff to me. I tried to find some clippy lints for that purpose, but have not found any yet. A similar issue exists [1], but has been kept open for >6 years and is not exactly what we want. [1] https://github.com/rust-lang/rust-clippy/issues/3117 -- Best Regards Junjie Mao >> I agree that uses of proc macros should be carefully evaluated to see if >> they really help or hurt. In this specific case, I'm not sure if using >> attributes solves the root cause, though. > > Yes, it doesn't help if you have multiple similarly-named methods > across the "superclasses". > > Paolo
