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

[Michael S. Tsirkin]

On Mon, Aug 13, 2012 at 03:57:41PM -0500, Anthony Liguori wrote:
> "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.

Not performance and not readability.  It boils down to not introducing
bugs.  Build failures on bugs are better than runtime ones.

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

It's a QOM bug that it wants zero offset, but since it does,
why doesn't QOM *check* zero offset? It should just fail build
if it isn't.

> 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

Not if you don't call 100% of functions.

> >
> > -- 
> > MST