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

[Marc-André Lureau]

Hi

On Wed, Nov 20, 2019 at 8:25 PM Alex Bennée <address@hidden> wrote:
>
>
> Philippe Mathieu-Daudé <address@hidden> writes:
>
> > 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);
>
> Using *s for the contextually appropriate state holding structure is
> certainly common enough in the code base. Maybe we should should
> document that too?

Yes, "s" is very common in qemu. Yet, it's ambiguous to me, as it is
used in other cases, while "self" is explicit for OOP-style, to refer
to the instance type being implemented.

>
> >     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'?
>
> I would have read it as:
>
>   SysBusDevice *self = SYS_BUS_DEVICE(obj);
>
> as the only device object in the example. But perhaps this is a complex
> example?
>
> >
> > 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";
> > }
> >
> > Maybe we should restrict 'self' as name of Object type only?
> > But your example is with SerialMM, so no?
>
>
> --
> Alex Bennée
>

thanks