[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] qdev property: cleanup
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH] qdev property: cleanup |
Date: |
Fri, 08 Apr 2016 13:17:51 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
[Wasn't delivered correctly by eggs.gnu.org, resending]
The subsystem tag should be just "qdev:". Suggest "qdev: Clean up
around properties".
Cao jin <address@hidden> writes:
> include:
> 1. remove unnecessary declaration of static function
> 2. fix inconsistency between comment and function name, and typo OOM->QOM
> 2. update comment of function
>
> Signed-off-by: Cao jin <address@hidden>
> ---
> A question about legacy property: I don`t see legacy property is really
> used in the code, so, why still add legacy property to object?
>
> since it has no maintainer, so the cc list is by intuition...
>
> hw/core/qdev.c | 14 ++++++--------
> include/hw/qdev-properties.h | 4 ++--
> 2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index db41aa1..743b71b 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -58,9 +58,6 @@ const char *qdev_fw_name(DeviceState *dev)
> return object_get_typename(OBJECT(dev));
> }
>
> -static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
> - Error **errp);
> -
> static void bus_remove_child(BusState *bus, DeviceState *child)
> {
> BusChild *kid;
> @@ -908,12 +905,12 @@ static void qdev_get_legacy_property(Object *obj,
> Visitor *v,
> }
>
> /**
> - * @qdev_add_legacy_property - adds a legacy property
> + * @qdev_property_add_legacy - adds a legacy property
Style#1:
function_name - headline
We generally use the imperative mood: "add a legacy property".
> *
> * Do not use this is new code! Properties added through this interface will
> * be given names and types in the "legacy" namespace.
> *
> - * Legacy properties are string versions of other OOM properties. The format
> + * Legacy properties are string versions of other QOM properties. The format
> * of the string depends on the property type.
> */
> static void qdev_property_add_legacy(DeviceState *dev, Property *prop,
> @@ -937,10 +934,11 @@ static void qdev_property_add_legacy(DeviceState *dev,
> Property *prop,
> }
>
> /**
> - * @qdev_property_add_static - add a @Property to a device.
> + * @qdev_property_add_static
> + * add a QOM property to @dev via its qdev Property @prop
Style#2:
function_name
headline
> *
> - * Static properties access data in a struct. The actual type of the
> - * property and the field depends on the property type.
> + * Static properties access data in a struct. The actual type of
> ObjectProperty
> + * and the struct field depends on the @qtype type.
> */
Not sure this part is an improvement. What's wrong with the current text?
> void qdev_property_add_static(DeviceState *dev, Property *prop,
> Error **errp)
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 0586cac..8c4481b 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -197,8 +197,8 @@ void error_set_from_qdev_prop_error(Error **errp, int
> ret, DeviceState *dev,
> Property *prop, const char *value);
>
> /**
> - * @qdev_property_add_static - add a @Property to a device referencing a
> - * field in a struct.
> + * @qdev_property_add_static - add a ObjectProperty to @dev via its qdev
> + * property @prop, referencing a field in the struct.
> */
Style#3:
function_name - sentence spanning multiple
lines.
Pick one style and stick to it.
The rest of qdev-properties.h appears to use GTK-Doc style (which I
personally dislike, but that doesn't matter here). See
https://developer.gnome.org/gtk-doc-manual/stable/documenting_symbols.html
If both declaration and definition have a comment, as they do here, they
should match exactly.
> void qdev_property_add_static(DeviceState *dev, Property *prop, Error
> **errp);
- Re: [Qemu-devel] [PATCH] qdev property: cleanup,
Markus Armbruster <=