Re: [PATCH] RFC: CODING_STYLE: describe "self" variable convention

[Marc-André Lureau]

Hi

On Wed, Nov 20, 2019 at 8:05 PM Philippe Mathieu-Daudé
<address@hidden> wrote:
>
> On 11/20/19 1:54 PM, Marc-André Lureau wrote:
> > Following the discussion in thread "[PATCH v3 13/33] serial: start
> > making SerialMM a sysbus device", I'd like to recommend the usage of
> > "self" variable to reference to the OOP-style method instance, as
> > commonly done in various languages and in GObject world.
> >
> > Cc: Peter Maydell <address@hidden>
> > Cc: Daniel P. Berrangé <address@hidden>
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >   CODING_STYLE.rst | 38 ++++++++++++++++++++++++++++++++------
> >   1 file changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/CODING_STYLE.rst b/CODING_STYLE.rst
> > index 427699e0e4..cb6635af71 100644
> > --- a/CODING_STYLE.rst
> > +++ b/CODING_STYLE.rst
> > @@ -102,12 +102,38 @@ Rationale:
> >   Naming
> >   ======
> >
> > -Variables are lower_case_with_underscores; easy to type and read.  
> > Structured
> > -type names are in CamelCase; harder to type but standing out.  Enum type
> > -names and function type names should also be in CamelCase.  Scalar type
> > -names are lower_case_with_underscores_ending_with_a_t, like the POSIX
> > -uint64_t and family.  Note that this last convention contradicts POSIX
> > -and is therefore likely to be changed.
> > +Variables are lower_case_with_underscores; easy to type and read.
> > +
> > +The most common naming for a variable is an abbreviation of the type
> > +name.  Some common examples:
> > +
> > +.. code-block:: c
> > +
> > +    Object *obj;
> > +    QVirtioSCSI *scsi;
> > +    SerialMM *smm;
> > +
> > +When writing QOM/OOP-style function, a "self" variable allows to refer
> > +without ambiguity to the instance of the method that is being
> > +implemented (this is not very common in QEMU code base, but it is
> > +often a good option to increase the readability and consistency,
> > +making further refactoring easier as well).  Example:
> > +
> > +.. code-block:: c
> > +
> > +    serial_mm_flush(SerialMM *self);
> > +
> > +    serial_mm_instance_init(Object *o) {
> > +        SerialMM *self = SERIAL_MM(o);
> > +        ..
> > +    }
> > +
> > +Structured type names are in CamelCase; harder to type but standing
> > +out.  Enum type names and function type names should also be in
> > +CamelCase.  Scalar type names are
> > +lower_case_with_underscores_ending_with_a_t, like the POSIX uint64_t
> > +and family.  Note that this last convention contradicts POSIX and is
> > +therefore likely to be changed.
> >
> >   When wrapping standard library functions, use the prefix ``qemu_`` to 
> > alert
> >   readers that they are seeing a wrapped version; otherwise avoid this 
> > prefix.
> >
>
> So in this example:
>
> static void pci_unin_agp_init(Object *obj)
> {
>      UNINHostState *s = UNI_NORTH_AGP_HOST_BRIDGE(obj);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>      PCIHostState *h = PCI_HOST_BRIDGE(obj);
>
>      /* Uninorth AGP bus */
>      memory_region_init_io(&h->conf_mem, OBJECT(h),
>                            &pci_host_conf_le_ops,
>                            obj, "unin-agp-conf-idx", 0x1000);
>      memory_region_init_io(&h->data_mem, OBJECT(h),
>                            &pci_host_data_le_ops,
>                            obj, "unin-agp-conf-data", 0x1000);
>
>      object_property_add_link(obj, "pic", TYPE_OPENPIC,
>                               (Object **) &s->pic,
>                               qdev_prop_allow_set_link_before_realize,
>                               0, NULL);
>
>      sysbus_init_mmio(sbd, &h->conf_mem);
>      sysbus_init_mmio(sbd, &h->data_mem);
> }
>
> You would change 'Object *obj' -> 'Object *self'?

static void pci_unin_agp_init(Object *obj)
{
  UNINHostState *self = UNI_NORTH_AGP_HOST_BRIDGE(obj);
  ..


When you override a parent method, you can create aliases for the
parent types with the abbreviation rule if necessary. But often, there
are less references to the parent types than the actual type you
implement, so in many cases, PARENT(self) can be less confusing. Your
example is a perhaps a bit more complicated than usual. Yet, having
"self" for the type we are implementing is still more readable to me.

>
> But here we want to keep 'klass', right?
>
> static void gpex_host_class_init(ObjectClass *klass, void *data)
> {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
>
>      hc->root_bus_path = gpex_host_root_bus_path;
>      dc->realize = gpex_host_realize;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      dc->fw_name = "pci";
> }


If we follow a similar rule for class methods, I would suggest:

static void gpex_host_class_init(ObjectClass *oc, void *data)
{
  GPEXClass *klass = GPEX_CLASS(oc);

But in general, class_init has more code dealing with various parent
types, to override parent methods.

>
> Maybe we should restrict 'self' as name of Object type only?
> But your example is with SerialMM, so no?
>

"self" for the instance type, "klass" for the the class type.