On 06/09/2017 04:10 AM, David Gibson wrote: > 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 <c...@kaod.org> 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.
ok. I can agree with that. > In this case, I intend to take the macro patch, mostly just on the > basis of avoiding further delays to rework the remaining patches. But I don't think having two different defines is a good idea : +#define ICP_PROP_XICS "xics" +#define ICS_PROP_XICS "xics" C.