Re: [Qemu-devel] [PATCH qom-next v2 3/5] target-arm: Use parent classes for reset + realize

[Peter Crosthwaite]

On Thu, Jul 11, 2013 at 8:36 PM, Andreas Färber <address@hidden> wrote:
> Am 11.07.2013 12:31, schrieb Michael S. Tsirkin:
>> On Thu, Jul 11, 2013 at 11:47:59AM +0200, Igor Mammedov wrote:
>>> On Thu, 11 Jul 2013 11:47:16 +1000
>>> address@hidden wrote:
>>>
>>>> From: Peter Crosthwaite <address@hidden>
>>>>
>>>> ARMCPUClass is only needed for parent-class abstract function access.
>>>> Just use parent classes for reset and realize access and remove
>>>> ARMCPUClass completely.
>>>>
>>>> Signed-off-by: Peter Crosthwaite <address@hidden>
>>>> ---
>>>>
>>>>  target-arm/cpu-qom.h | 20 --------------------
>>>>  target-arm/cpu.c     | 16 +++++++---------
>>>>  2 files changed, 7 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
>>>> index ef6261f..bdad93a 100644
>>>> --- a/target-arm/cpu-qom.h
>>>> +++ b/target-arm/cpu-qom.h
>>>> @@ -24,28 +24,8 @@
>>>>
>>>>  #define TYPE_ARM_CPU "arm-cpu"
>>>>
>>>> -#define ARM_CPU_CLASS(klass) \
>>>> -    OBJECT_CLASS_CHECK(ARMCPUClass, (klass), TYPE_ARM_CPU)
>>>>  #define ARM_CPU(obj) \
>>>>      OBJECT_CHECK(ARMCPU, (obj), TYPE_ARM_CPU)
>>>> -#define ARM_CPU_GET_CLASS(obj) \
>>>> -    OBJECT_GET_CLASS(ARMCPUClass, (obj), TYPE_ARM_CPU)
>>>> -
>>>> -/**
>>>> - * ARMCPUClass:
>>>> - * @parent_realize: The parent class' realize handler.
>>>> - * @parent_reset: The parent class' reset handler.
>>>> - *
>>>> - * An ARM CPU model.
>>>> - */
>>>> -typedef struct ARMCPUClass {
>>>> -    /*< private >*/
>>>> -    CPUClass parent_class;
>>>> -    /*< public >*/
>>>> -
>>>> -    DeviceRealize parent_realize;
>>>> -    void (*parent_reset)(CPUState *cpu);
>>>> -} ARMCPUClass;
>>>>
>>>>  /**
>>>>   * ARMCPU:
>>>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>>>> index ed53df8..ad5ec7b 100644
>>>> --- a/target-arm/cpu.c
>>>> +++ b/target-arm/cpu.c
>>>> @@ -60,7 +60,8 @@ static void cp_reg_reset(gpointer key, gpointer value, 
>>>> gpointer opaque)
>>>>  static void arm_cpu_reset(CPUState *s)
>>>>  {
>>>>      ARMCPU *cpu = ARM_CPU(s);
>>>> -    ARMCPUClass *acc = ARM_CPU_GET_CLASS(cpu);
>>>> +    CPUClass *cc_parent =
>>>> +            CPU_CLASS(object_class_get_parent_by_name(TYPE_ARM_CPU));
>>> Maybe object_class_get_parent_of_type() would be less confusing?
>>>
>>> This usage assumes that parent of TYPE_ARM_CPU is TYPE_CPU and if
>>> another TYPE_X added between them, it might break if TYPE_X doesn't
>>> re-implement this logic in its reset.
>>
>> If what's needed is TYPE_CPU, you can just look up the class by name.
>

That means the function has knowedge of who the direct parent is which
I am trying to avoid. The goal is the Device::realize function knows
it has to call the parent version. As for who that direct parent is,
that ideally remain abstracted away from here. If you directly
reference a parent class by name you also make it hard to change an
objects parent (which is currently only defined in one place.

> My suggestion was to hide the implementation details behind an
> ARM_CPU_GET_PARENT_CLASS(obj) macro.
>

Respinning. as such. I think this is a good idea. Looks more
consistent with neighbouring code as well.

Regards,
Peter