From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45568) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SmmKl-0007Eb-Nu for qemu-devel@nongnu.org; Thu, 05 Jul 2012 09:43:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SmmKa-0002nr-85 for qemu-devel@nongnu.org; Thu, 05 Jul 2012 09:43:03 -0400 Message-ID: <4FF599D6.1070005@suse.de> Date: Thu, 05 Jul 2012 15:42:46 +0200 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1341422373-13614-1-git-send-email-afaerber@suse.de> <1341422373-13614-15-git-send-email-afaerber@suse.de> <20120704211717.GC27653@redhat.com> <20120704212614.GA27711@redhat.com> <4FF4C4EC.2090404@suse.de> <20120705133431.GF30572@redhat.com> In-Reply-To: <20120705133431.GF30572@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 14/14] pci: Tidy up PCI host bridges List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: jbaron@redhat.com, qemu-devel@nongnu.org, Alexander Graf , =?ISO-8859-1?Q?Andrea?= =?ISO-8859-1?Q?s_F=E4rber?= , "\"list@suse.de@zmta06.collab.prod.int.phx2.redhat.com\"@suse.de:New World" , anthony@codemonkey.ws, pbonzini@redhat.com, open@suse.de Am 05.07.2012 15:34, schrieb Michael S. Tsirkin: > On Thu, Jul 05, 2012 at 12:34:20AM +0200, Andreas F=E4rber wrote: >> Am 04.07.2012 23:26, schrieb Michael S. Tsirkin: >>> On Thu, Jul 05, 2012 at 12:17:17AM +0300, Michael S. Tsirkin wrote: >>>> On Wed, Jul 04, 2012 at 07:19:33PM +0200, Andreas F=E4rber wrote: >>>>> Uglify the parent field to enforce QOM-style access via casts. >>>>> Don't just typedef PCIHostState, either use it directly or embed it= . >>>>> >>>>> Signed-off-by: Andreas F=E4rber >>>>> --- >>>>> hw/alpha_typhoon.c | 4 ++-- >>>>> hw/dec_pci.c | 2 +- >>>>> hw/grackle_pci.c | 2 +- >>>>> hw/gt64xxx.c | 26 ++++++++++++++++---------- >>>>> hw/piix_pci.c | 6 ++++-- >>>>> hw/ppc4xx_pci.c | 8 +++++--- >>>>> hw/ppce500_pci.c | 2 +- >>>>> hw/prep_pci.c | 8 +++++--- >>>>> hw/spapr_pci.c | 12 +++++++----- >>>>> hw/spapr_pci.h | 2 +- >>>>> hw/unin_pci.c | 14 +++++++------- >>>>> 11 files changed, 50 insertions(+), 36 deletions(-) >>>>> >>>>> diff --git a/hw/alpha_typhoon.c b/hw/alpha_typhoon.c >>>>> index 58025a3..955d628 100644 >>>>> --- a/hw/alpha_typhoon.c >>>>> +++ b/hw/alpha_typhoon.c >>>>> @@ -46,7 +46,7 @@ typedef struct TyphoonPchip { >>>>> OBJECT_CHECK(TyphoonState, (obj), TYPE_TYPHOON_PCI_HOST_BRIDGE= ) >>>>> =20 >>>>> typedef struct TyphoonState { >>>>> - PCIHostState host; >>>>> + PCIHostState parent_obj; >>>>> =20 >>>>> TyphoonCchip cchip; >>>>> TyphoonPchip pchip; >>>>> @@ -770,7 +770,7 @@ PCIBus *typhoon_init(ram_addr_t ram_size, ISABu= s **isa_bus, >>>>> b =3D pci_register_bus(dev, "pci", >>>>> typhoon_set_irq, sys_map_irq, s, >>>>> &s->pchip.reg_mem, addr_space_io, 0, 64); >>>>> - s->host.bus =3D b; >>>>> + PCI_HOST_BRIDGE(s)->bus =3D b; >>>>> =20 >>>>> /* Pchip0 PCI special/interrupt acknowledge, 0x801.F800.0000, = 64MB. */ >>>>> memory_region_init_io(&s->pchip.reg_iack, &alpha_pci_iack_ops,= b, >>>> >>>> Sorry I don't understand. >>>> why are we making code ugly apparently intentionally? >>> >>> Just to clarify: replacing upcasts which are always safe >>> with downcasts which can fail is what I consider especially ugly. >> >> As per Anthony the parent field in the QOM instance structs is not >> supposed to be touched (cf. object.h). We mark it /*< private >*/ so >> that it doesn't even show up in gtk-doc documentation. >=20 > PCIHostState is not private here. And if it were, it won't be > of any use because compiler does not read gtk-doc documentation > and neither do developers. >=20 >> If it is unused, >> its name becomes irrelevant and could even be "reserved" if we so >> wanted. Renaming it to whatever proves that all old references are gon= e. >=20 > Adding arbitrary rules like that only seems to make code future proof. > People will not remember and will use the field anyway, > then when you try to change it there will be work to be done. > So let's do the work when we really need it. >=20 >> Background is that qdev and QOM work differently with regards to >> inheritance: as mentioned in the preceding patch, for qdev the parent >> was (had to be) identified by name and could be anywhere in the struct= ; >> for QOM the parent is a subset of the struct from the start and it's >> supposed to be accessed through the struct type that provides the >> fields, the usual way to get such a pointer is through >> OBJECT_CHECK()-derived cast macros. >=20 > It makes sense if you go from parent to child. Even in C++ > you don't use dynamic_cast to go from child to parent. Which is the core point: In C++ you don't need to go through some parent field, you access the fields and methods directly through the child variable. In C that's not possible. Sticking to the specific type that has the fields you want to access allows you to code C++-style. Anyway, I am not doing these changes here because I have great arguments foo and bar for it but because this is QOM that so far no one opposed when Anthony introduced it. Please discuss that with Anthony. My interest is having it done consistently and not having legacy code lying around that gets copied by new contributors. Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg