Re: vtables and procedural macros (was Re: [PATCH] rust: pl011: convert pl011_create to safe Rust)

[Junjie Mao]

Junjie Mao <junjie.mao@hotmail.com> writes:

> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On Tue, Feb 11, 2025 at 7:47 AM Junjie Mao <junjie.mao@hotmail.com> 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.

Just found the lint: 
https://rust-lang.github.io/rust-clippy/master/index.html#same_name_method

>
> [1] https://github.com/rust-lang/rust-clippy/issues/3117

--
Best Regards
Junjie Mao