Re: [Qemu-devel] [PATCH] qom: add style guide

[Anthony Liguori]

"Michael S. Tsirkin" <address@hidden> writes:

> On Mon, Aug 13, 2012 at 01:46:46PM -0500, Anthony Liguori wrote:
>> +    typedef struct MyType MyType;
>> +    
>> +    struct MyType
>> +    {
>
> This seems to violate our style:should be
>
>> +    struct MyType {

That's a bug in CODING_STYLE.  Coding style only talks explicitly about
if but ought to make an exception for type declarations too.  If you
grep a bit, you'll see both styles are wildly used.

>> +        Object parent_obj;
>> +    
>> +        /*< private >*/
>> +        int foo;
>> +    };
>> +
>> +When declaring the structure, a forward declaration should be used.  This is
>> +useful for consistency sake as it is required when defining classes.
>> +
>> +The first element must be the parent type and should be named 'parent_obj' 
>> or
>> +just 'parent'.
>
> Why should it? Why not use a descriptive name that
> makes it easier to see what the object actually is?

Parent is a descriptive name.  That's all it is--the parent.  It is not
'bus' or 'bridge' or anything else you want to call it.  It's the parent
object.

If you want to interact with the object as the parent, you should cast.

>>  When working with QOM types, you should avoid ever accessing
>> +this member directly instead relying on casting macros.
>> +
>> +Casting macros hide the inheritence hierarchy from the implementation.  This
>> +makes it easier to refactor code over time by changing the hierarchy without
>> +changing the code in many places.
>
> This seems like a weak motivation. Why do you expect to refactor
> hierarchy all the time?  The cost is replacing compile time checks with
> runtime ones.

Unless you have a case where runtime checks have a measurable cost associated
with them, an appeal to performance is not valid.

It simply boils down to readability.

struct PCIDevice
{
    DeviceState qdev; // what do we call this?
};

struct E1000
{
    PCIDevice pci_dev;
};

E1000 *s = ...;

device_reset(&s->pci_dev->qdev);

Is not at all descriptive.  It's also hard to review for when people
introduce types like this.  And it's not clear why you can only have one
PCIDevice member.  Why isn't:

struct E1000
{
    PCIDevice pci_dev0;
    PCIDevice pci_dev1;
};

Not valid?  It's not obvious to a casual observer.  Using a name other
than 'parent' just allows people to have the wrong mental model.  It is
not a has-a relationship.  s->pci_dev leads a reader to think of things
in terms of a has-a relationship.  It's an is-a relationship.

> So refactoring is easier to make but harder to make correct.
> Sounds like a bad tradeoff.

99% of the work in introducing QOM was cleaning up direct references to
members by wrapping them in functions.

It's pretty darn hard to misuse cast macros.  I don't buy that they are
any less correct in practice.  Casting is done usually at the top of a
function and is unconditional.  Even the most basic testing should cover
100% of casts.

Regards,

Anthony Liguori

>
> -- 
> MST