[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc |
Date: |
Fri, 15 Feb 2013 13:48:31 +0000 |
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
- [Qemu-devel] [PATCH 0/5] Remove sysbus_add_memory and sysbus_del_memory, Peter Maydell, 2013/02/15
- [Qemu-devel] [PATCH 1/5] sysbus: make SysBusDeviceClass::init optional, Peter Maydell, 2013/02/15
- [Qemu-devel] [PATCH 5/5] sysbus: Remove sysbus_add_memory and sysbus_del_memory, Peter Maydell, 2013/02/15
- [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc, Peter Maydell, 2013/02/15
- Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc, Andreas Färber, 2013/02/15
- Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc, Peter Maydell, 2013/02/15
- Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc, Andreas Färber, 2013/02/15
- Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc,
Peter Maydell <=
- Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc, Paolo Bonzini, 2013/02/15
- Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc, Andreas Färber, 2013/02/15
- Re: [Qemu-devel] [PATCH 2/5] musicpal: qdevify musicpal-misc, Peter Maydell, 2013/02/15
[Qemu-devel] [PATCH 3/5] milkymist-minimac2: Just expose buffers as a sysbus mmio region, Peter Maydell, 2013/02/15
[Qemu-devel] [PATCH 4/5] milkymist-softusb: Don't map RAM memory regions in the device itself, Peter Maydell, 2013/02/15