Philippe Mathieu-Daudé <phi...@redhat.com> 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 <peter.mayd...@linaro.org>
>> Cc: Daniel P. Berrangé <berra...@redhat.com>
>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> ---
>> 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?
> 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
