[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qom: add style guide
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] qom: add style guide |
Date: |
Tue, 14 Aug 2012 00:57:35 +0300 |
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