Re: [RFC PATCH v3 13/14] qemu/target_info: Add target_aarch64() helper

[Markus Armbruster <armbru@redhat.com>]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 4/21/25 23:04, Markus Armbruster wrote:
>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>> [...]
>> 
>>> At this point, I would like to focus on having a first version of TargetInfo API, and not reviewing any other changes, as things may be modified, and they would need to be reviewed again. It's hard to follow the same abstraction done multiple times in multiple series.
>>>
>>> Regarding your proposal for target_system_arch(), I understand that you tried to reuse the existing SysEmuTarget, which was a good intention.
>>> However, I don't think we should use any string compare for this (which qemu_api_parse does). It has several flaws:
>> 
>> qemu_api_parse()?  Do you mean qapi_enum_parse()?
>
> Yes, sorry for the typo.
>
>>> - The most important one: it can fail (what if -1 is returned?). Enums can be guaranteed and exhaustive at compile time.
>>> - It's slower than having the current arch directly known at compile time.
>>> As well, since SysEmuTarget is a generated enum, it makes it much harder to follow code IMHO.
>>> QAPI requires those things to be defined from a json file for external usage, but it's not a good reason for being forced to use it in all the codebase as the only possible abstraction.
>>>
>>> To have something fast and infallible, we can adopt this solution:
>>>
>>> In target_info.h:
>>>
>>> /* Named TargetArch to not clash with poisoned TARGET_X */
>>> typedef enum TargetArch {
>>>      TARGET_ARCH_AARCH64,
>>>      TARGET_ARCH_ALPHA,
>>>      TARGET_ARCH_ARM,
>>>      TARGET_ARCH_AVR,
>>>      TARGET_ARCH_HPPA,
>>>      TARGET_ARCH_I386,
>>>      TARGET_ARCH_LOONGARCH64,
>>>      TARGET_ARCH_M68K,
>>>      TARGET_ARCH_MICROBLAZE,
>>>      TARGET_ARCH_MICROBLAZEEL,
>>>      TARGET_ARCH_MIPS,
>>>      TARGET_ARCH_MIPS64,
>>>      TARGET_ARCH_MIPS64EL,
>>>      TARGET_ARCH_MIPSEL,
>>>      TARGET_ARCH_OR1K,
>>>      TARGET_ARCH_PPC,
>>>      TARGET_ARCH_PPC64,
>>>      TARGET_ARCH_RISCV32,
>>>      TARGET_ARCH_RISCV64,
>>>      TARGET_ARCH_RX,
>>>      TARGET_ARCH_S390X,
>>>      TARGET_ARCH_SH4,
>>>      TARGET_ARCH_SH4EB,
>>>      TARGET_ARCH_SPARC,
>>>      TARGET_ARCH_SPARC64,
>>>      TARGET_ARCH_TRICORE,
>>>      TARGET_ARCH_X86_64,
>>>      TARGET_ARCH_XTENSA,
>>>      TARGET_ARCH_XTENSAEB,
>>> } TargetArch;
>> 
>> Effectively duplicates generated enum SysEmuTarget.  Can you explain why
>> that's useful?
>
> In terms of code, it doesn't change anything. we could reuse SysEmuTarget.
> I just think it's more clear to have the enum defined next to the structure using it, compared to have this buried in generated code that can't be grepped easily.
> (git grep SYS_EMU returns nothing, so people have to remember it's converted from a Json, and that naming is different).

Yes, that's unfortunate, but we don't duplicate other QAPI enums because
of it.

> IMHO, DRY principle should not always be followed blindly when it hurts code readability.

Treating DRY as dogma would be foolish.

> That said, my editor is able to find the generated definition as well, so I don't really mind reusing SysEmuTarget if we think the code readability is not worth the duplication.

It's not just the duplication, it's also the conversion between the two
enums.

> However, I think it's a problem to compare strings to find this, when it can be set at compile time, in a file that will have to stay target specific anyway.

Converting from string to enum at first practical opportunity makes the
code simpler more often than not.


[...]