Re: [Qemu-ppc] [PATCH v4 1/6] xics: introduce macros for ICP/ICS link properties

[David Gibson]

On Thu, Jun 08, 2017 at 07:26:35PM +0200, Cédric Le Goater wrote:
> On 06/08/2017 07:00 PM, Greg Kurz wrote:
> > On Thu, 8 Jun 2017 18:08:44 +0200
> > Cédric Le Goater <address@hidden> wrote:
> > 
> >>>>> FWIW, other people do that as well (see hw/i386/pc_q35.c for example).  
> >>>>>   
> >>>>
> >>>> well, I don't see the benefits of changing a string constant by a 
> >>>> define. 
> >>>>  
> >>>
> >>> Improved semantics,  especially since the "xics" string appears in 
> >>> many places with different meanings.   
> >>
> >> ah ? If so, we should do a cleanup up. The code seems consistent from 
> >> what I can see. xics is a general name for :
> >>
> >>    'PowerPC interrupt controller (type 2)' 
> >>
> >> and it is mostly used as a prefix. There are no "xics" object, only a 
> > 
> > I'm only talking about "xics" as a property name actually:
> > 
> > $ git grep '"xics"'
> > hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", 
> > &err);
> > hw/intc/xics.c:    obj = object_property_get_link(OBJECT(dev), "xics", 
> > &err);
> > hw/ppc/pnv.c:    object_property_add_const_link(OBJECT(&chip->psi), "xics",
> > hw/ppc/pnv.c:        object_property_add_const_link(OBJECT(pnv_core), 
> > "xics",
> > hw/ppc/pnv_core.c:    object_property_add_const_link(obj, "xics", 
> > OBJECT(xi), &error_abort);
> > hw/ppc/pnv_core.c:    xi = object_property_get_link(OBJECT(dev), "xics", 
> > &local_err);
> > hw/ppc/pnv_psi.c:    obj = object_property_get_link(OBJECT(dev), "xics", 
> > &err);
> > hw/ppc/pnv_psi.c:    object_property_add_const_link(OBJECT(ics), "xics", 
> > obj,  &error_abort);
> > hw/ppc/spapr.c:    object_property_add_const_link(obj, "xics", 
> > OBJECT(spapr), &error_abort);
> > hw/ppc/spapr_cpu_core.c:    object_property_add_const_link(obj, "xics", 
> > OBJECT(spapr), &error_abort);
> > 
> > You have to read the code to know which ones are related.
> 
> The "xics" property link always point to the same object : 
> the XICSFabric object which is the machine, spapr or pnv. 
> 
> > With this patch applied, it is mostly obvious, even for the newbie:
> 
> ah. the goal is to know where in the code the link was set. 
> It can be even more complex with aliases.

There doesn't seem to be a strong convention about whether to use raw
property names or defines across qemu.  I'm not all that fussed either
way.

I do see one small advantage to use defines: if you make a typo, it
will probably result in a compile time error, whereas with a bare
string it won't show up until a runtime error.

In this case, I intend to take the macro patch, mostly just on the
basis of avoiding further delays to rework the remaining patches.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature