qemu-devel
[Top][All Lists]
Advanced

[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);



reply via email to

[Prev in Thread] Current Thread [Next in Thread]