Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc

[Andreas Färber]

Am 15.02.2013 14:38, schrieb Peter Maydell:
> On 15 February 2013 13:11, Andreas Färber <address@hidden> wrote:
>> Am 15.02.2013 12:45, schrieb Peter Maydell:
>>> --- a/hw/musicpal.c
>>> +++ b/hw/musicpal.c
>>> @@ -1031,6 +1031,21 @@ static const TypeInfo mv88w8618_flashcfg_info = {
>>>
>>>  #define MP_BOARD_REVISION       0x31
>>>
>>> +typedef struct {
>>
>> Anonymous struct
> 
> No, it's a typedef. Why would you want to name the struct
> particularly when it's an error to then use that name rather
> than the typedef? Better to let the compiler make uses of
> 'struct Foo' rather than 'Foo' a compile failure.

I'm pretty sure it has been requested by Blue and Anthony in the past...
Not sure if it makes a difference for gdb or something.

> 
>>> +    SysBusDevice busdev;
>>
>> parent_obj please. :)
>>
>>> +    MemoryRegion iomem;
>>> +} MusicPalMiscState;
>>> +
>>
>>> +typedef SysBusDeviceClass MusicPalMiscClass;
>>
>> Please don't. Either define a struct and use it for .class_size or drop
>> the typedef.
> 
> So my rationale here was "all classes should have a FooClass".
> If you have classes which don't have a FooClass then if at
> some later point you need to introduce a class struct you
> have to go round and locate all the places you wrote
> ParentClass and now need to change it to FooClass. If
> we always have a typedef everywhere then there is never
> a problem.
> 
> More generally, I think we should prefer to avoid the kind of
> shortcut that the C implementation allows us to take. If we
> define a QOM class then that should mean you get the full range
> of expected things (a TYPE_FOO, a FOO macro, a FOO_CLASS and
> a FooClass type).
> 
> If you prefer we could standardize on
>   typedef struct {
>       ParentClass parent;
>   } FooClass;
> 
> rather than typedef ParentClass FooClass;
> 
>>> +
>>> +#define TYPE_MUSICPAL_MISC "musicpal-misc"
>>> +#define MUSICPAL_MISC(obj) \
>>> +     OBJECT_CHECK(MusicPalMiscState, (obj), TYPE_MUSICPAL_MISC)
>>
>>> +#define MUSICPAL_MISC_CLASS(klass) \
>>> +     OBJECT_CLASS_CHECK(MusicPalMiscClass, (klass), TYPE_MUSICPAL_MISC)
>>> +#define MUSICPAL_MISC_GET_CLASS(obj) \
>>> +     OBJECT_GET_CLASS(MusicPalMiscClass, (obj), TYPE_MUSICPAL_MISC)
>>
>> If we don't have such a class so you can just drop these two.
>> SYS_BUS_DEVICE_[GET_]CLASS() should be used instead when needed.
> 
> Again, this will then require rework if we ever actually need
> to put anything in the currently (conceptually) empty class struct.

It may need rework either way. Because aliasing via typedef gives
FooClass fields it will loose once it is turned into a real QOM class.
We had such an issue with i440fx in my PHB series, that's why I'm
sensitive to it. ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg