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

[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.

>> +    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.

-- PMM