Pierrick Bouvier <pierrick.bouv...@linaro.org> writes:
> On 4/21/25 23:04, Markus Armbruster wrote:
>> Pierrick Bouvier <pierrick.bouv...@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.
[...]
