Re: [RFC PATCH v5 08/21] hw/arm: Add DEFINE_MACHINE_[ARM_]AARCH64() macros

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 4/28/25 11:44 AM, BALATON Zoltan wrote:
> On Mon, 28 Apr 2025, Pierrick Bouvier wrote:
>> On 4/28/25 3:31 AM, BALATON Zoltan wrote:
>>> Since you are touching the lines using DEFINE_MACHINE it's a good
>>> opportunity to change the macro to be more general to be able to keep
>>> using it instead of replacing it with the boiler plate it's supposed to
>>> hide. Adding one or two more parameters to the macro is not a big change
>>> so I don't see why you don't want to do it. This could be addressed later
>>> to revert to use the macro again but in practice it will not be addressed
>>> because everybody will be busy doing other things and doing that now would
>>> prevent some churn. I too, don't like doing unrelated clean up which is
>>> not the main goal, but if it's not much more work then it's not
>>> unreasonable to do it. I only oppose to that if it's a lot of work so I
>>> would not ask such change but what I asked is not unrelated and quite
>>> simple change.
>>>
>>> That said, I can't stop you so if you still don't want to do it now then
>>> you can move on. I don't care that much as long as you stay within hw/arm,
>>> but will raise my concern again when you submit a similar patch that
>>> touches parts I care more about. If others don't think it's a problem and
>>> not bothered by the boiler plate code then it's not so important but
>>> otherwise I think I have a valid point. I remember when I started to get
>>> to know QEMU it was quite difficult to wade through all the QOM boiler
>>> plate just to see what is related to the actual functionality. These
>>> macros help to make code more readable and accessible for new people.
>>
>> Having been through that recently, I agree with you that it can be hard to
>> follow at first. Luckily, we have perfect compiler based completion for all
>> editors those days (I sincerely hope everyone spent 2 hours configuring this
>> on their own favorite one), and it's easy to see where things are defined and
>> used, even when code is cryptic.
> 
> It's not about typing but reading it. The verbose struct definitions are
> hard to follow and makes board code look more complex than it should be.
> 
>> That said, pushing to someone adding a new field the responsibility of
>> cleaning up the whole thing is not a fair request. You can't expect your
>> friends to clean your shared house because they brought a cake for dinner.
> 
> I tend to get such requests to clean up unrelated things whenever I try to
> change anything in PPC Mac emulation which I also complain about and think
> is not reasonable to ask. But I did not ask for unrelated cleanup here and
> changing the patch so you don't do this:
> 
> -DEFINE_MACHINE("none", machine_none_machine_init)
> +static const TypeInfo null_machine_types[] = {
> +    {
> +        .name           = MACHINE_TYPE_NAME("none"),
> +        .parent         = TYPE_MACHINE,
> +        .class_init     = null_machine_class_init,
> +    },
> +};
> +
> +DEFINE_TYPES(null_machine_types)
> 
> but instead add the .interfaces field to a variant of DEFINE_MACHINE once
> and keep the one line definition is not something unreasonable to ask. I
> think you can ask your friends to not make a mess in the shared house
> while having a party or at least clean up after that. Adding one more
> parameter to the macro is also simple to do so I don't get why you're so
> opposed to this.
>

Maybe there is a misunderstanding on my side, but it seems that what you 
asked is exactly patch 7, which introduce DEFINE_MACHINE_WITH_INTERFACES.

That said, patch 4 ("hw/core/null-machine: Define machine as generic QOM 
type") could use it to define the null machine. Philippe, could you 
change patch 4 to use DEFINE_MACHINE_WITH_INTERFACES instead?

Thanks,
Pierrick

> Regards,
> BALATON Zoltan