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

[Peter Maydell]

On 15 February 2013 13:45, Andreas Färber <address@hidden> wrote:
> 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.

I've cc'd them. But unless somebody has an actual good reason
for giving the struct in the typedef a completely unnecessary
name I think leaving it unnamed is better.
(This is distinct from genuinely anonymous structs with no
'struct foo' name or typedef name, which we do have a few of
in the codebase. I agree those are better avoided.)

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

> 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. ;)

OK, so that seems like an argument for always defining the
empty-except-for-the-parentclass class struct, or does that
run into problems too?

-- PMM