From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43762) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T12cm-0007M1-Kr for qemu-devel@nongnu.org; Mon, 13 Aug 2012 17:56:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T12ck-0006zT-DG for qemu-devel@nongnu.org; Mon, 13 Aug 2012 17:56:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52456) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T12ck-0006zM-5K for qemu-devel@nongnu.org; Mon, 13 Aug 2012 17:56:34 -0400 Date: Tue, 14 Aug 2012 00:57:35 +0300 From: "Michael S. Tsirkin" Message-ID: <20120813215735.GB15639@redhat.com> References: <1344883606-14854-1-git-send-email-aliguori@us.ibm.com> <20120813195529.GC19139@redhat.com> <87obmey0fu.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87obmey0fu.fsf@codemonkey.ws> Subject: Re: [Qemu-devel] [PATCH] qom: add style guide List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Peter Maydell , Paolo Bonzini , qemu-devel@nongnu.org, Andreas Faerber On Mon, Aug 13, 2012 at 03:57:41PM -0500, Anthony Liguori wrote: > "Michael S. Tsirkin" 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