Re: [Qemu-devel] [PATCH 1/9] char.h: fix gtk-doc comment style

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Use the gtk-doc function comment style, as documented in:
> https://developer.gnome.org/gtk-doc-manual/stable/documenting_symbols.html.en

I'm no friend of this style, but these headers use it already, except
they get it wrong in places.  Your subject's "fix" expresses that, but
then your commit message body suggests a conversion to gtk-doc style.
Suggest to replace it by

  Fix up conformance to GTK-Doc function comment style, as documented in
  https://developer.gnome.org/gtk-doc-manual/stable/documenting_symbols.html.en

>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/chardev/char-fe.h | 81 ++++++++++++++++++---------------------
>  include/chardev/char.h    | 61 +++++++++++++----------------
>  2 files changed, 63 insertions(+), 79 deletions(-)
>
> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
> index c67271f1ba..21071f1fb1 100644
> --- a/include/chardev/char-fe.h
> +++ b/include/chardev/char-fe.h
> @@ -20,7 +20,7 @@ struct CharBackend {
>  };
>  
>  /**
> - * @qemu_chr_fe_init:
> + * qemu_chr_fe_init:

Preexisting: fails to document parameters.  Leaving them undocumented is
acceptable for this style-fix patch.  More of the same below.

>   *
>   * Initializes a front end for the given CharBackend and
>   * Chardev. Call qemu_chr_fe_deinit() to remove the association and
> @@ -31,7 +31,7 @@ struct CharBackend {
>  bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp);
>  
>  /**
> - * @qemu_chr_fe_deinit:
> + * qemu_chr_fe_deinit:
>   * @b: a CharBackend
>   * @del: if true, delete the chardev backend
>  *
> @@ -42,9 +42,9 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error 
> **errp);
>  void qemu_chr_fe_deinit(CharBackend *b, bool del);
>  
>  /**
> - * @qemu_chr_fe_get_driver:
> + * qemu_chr_fe_get_driver:
>   *
> - * Returns the driver associated with a CharBackend or NULL if no
> + * Returns: the driver associated with a CharBackend or NULL if no
>   * associated Chardev.
>   * Note: avoid this function as the driver should never be accessed directly,
>   *       especially by the frontends that support chardevice hotswap.
> @@ -53,21 +53,21 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del);
>  Chardev *qemu_chr_fe_get_driver(CharBackend *be);
>  
>  /**
> - * @qemu_chr_fe_backend_connected:
> + * qemu_chr_fe_backend_connected:
>   *
> - * Returns true if there is a chardevice associated with @be.
> + * Returns: true if there is a chardevice associated with @be.
>   */
>  bool qemu_chr_fe_backend_connected(CharBackend *be);
>  
>  /**
> - * @qemu_chr_fe_backend_open:
> + * qemu_chr_fe_backend_open:
>   *
> - * Returns true if chardevice associated with @be is open.
> + * Returns: true if chardevice associated with @be is open.
>   */
>  bool qemu_chr_fe_backend_open(CharBackend *be);
>  
>  /**
> - * @qemu_chr_fe_set_handlers:
> + * qemu_chr_fe_set_handlers:
>   * @b: a CharBackend
>   * @fd_can_read: callback to get the amount of data the frontend may
>   *               receive
> @@ -95,7 +95,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
>                                bool set_open);
>  
>  /**
> - * @qemu_chr_fe_take_focus:
> + * qemu_chr_fe_take_focus:
>   *
>   * Take the focus (if the front end is muxed).
>   *
> @@ -104,14 +104,14 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
>  void qemu_chr_fe_take_focus(CharBackend *b);
>  
>  /**
> - * @qemu_chr_fe_accept_input:
> + * qemu_chr_fe_accept_input:
>   *
>   * Notify that the frontend is ready to receive data
>   */
>  void qemu_chr_fe_accept_input(CharBackend *be);
>  
>  /**
> - * @qemu_chr_fe_disconnect:
> + * qemu_chr_fe_disconnect:
>   *
>   * Close a fd accepted by character backend.
>   * Without associated Chardev, do nothing.
> @@ -119,7 +119,7 @@ void qemu_chr_fe_accept_input(CharBackend *be);
>  void qemu_chr_fe_disconnect(CharBackend *be);
>  
>  /**
> - * @qemu_chr_fe_wait_connected:
> + * qemu_chr_fe_wait_connected:
>   *
>   * Wait for characted backend to be connected, return < 0 on error or
>   * if no associated Chardev.

Preexisting: missing Returns:.  Again, leaving it that way is acceptable
for this style-fix patch.

> @@ -127,19 +127,18 @@ void qemu_chr_fe_disconnect(CharBackend *be);
>  int qemu_chr_fe_wait_connected(CharBackend *be, Error **errp);
>  
>  /**
> - * @qemu_chr_fe_set_echo:
> + * qemu_chr_fe_set_echo:
> + * @echo true to enable echo, false to disable echo

Colon after @echo, please.

>   *
>   * Ask the backend to override its normal echo setting.  This only really
>   * applies to the stdio backend and is used by the QMP server such that you
>   * can see what you type if you try to type QMP commands.
>   * Without associated Chardev, do nothing.
> - *
> - * @echo true to enable echo, false to disable echo
>   */
>  void qemu_chr_fe_set_echo(CharBackend *be, bool echo);
>  
>  /**
> - * @qemu_chr_fe_set_open:
> + * qemu_chr_fe_set_open:
>   *
>   * Set character frontend open status.  This is an indication that the
>   * front end is ready (or not) to begin doing I/O.
> @@ -148,83 +147,77 @@ void qemu_chr_fe_set_echo(CharBackend *be, bool echo);
>  void qemu_chr_fe_set_open(CharBackend *be, int fe_open);
>  
>  /**
> - * @qemu_chr_fe_printf:
> + * qemu_chr_fe_printf:
> + * @fmt see #printf

Likewise.

>   *
>   * Write to a character backend using a printf style interface.  This
>   * function is thread-safe. It does nothing without associated
>   * Chardev.
> - *
> - * @fmt see #printf
>   */
>  void qemu_chr_fe_printf(CharBackend *be, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
>  
>  /**
> - * @qemu_chr_fe_add_watch:
> + * qemu_chr_fe_add_watch:
> + * @cond the condition to poll for
> + * @func the function to call when the condition happens
> + * @user_data the opaque pointer to pass to @func

Likewise.

>   *
>   * If the backend is connected, create and add a #GSource that fires
>   * when the given condition (typically G_IO_OUT|G_IO_HUP or G_IO_HUP)
>   * is active; return the #GSource's tag.  If it is disconnected,
>   * or without associated Chardev, return 0.
>   *
> - * @cond the condition to poll for
> - * @func the function to call when the condition happens
> - * @user_data the opaque pointer to pass to @func
> - *
>   * Returns: the source tag
>   */
>  guint qemu_chr_fe_add_watch(CharBackend *be, GIOCondition cond,
>                              GIOFunc func, void *user_data);
>  
>  /**
> - * @qemu_chr_fe_write:
> + * qemu_chr_fe_write:
> + * @buf the data
> + * @len the number of bytes to send

Likewise.

>   *
>   * Write data to a character backend from the front end.  This function
>   * will send data from the front end to the back end.  This function
>   * is thread-safe.
>   *
> - * @buf the data
> - * @len the number of bytes to send
> - *
>   * Returns: the number of bytes consumed (0 if no associated Chardev)
>   */
>  int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len);
>  
>  /**
> - * @qemu_chr_fe_write_all:
> + * qemu_chr_fe_write_all:
> + * @buf the data
> + * @len the number of bytes to send

Likewise.

>   *
>   * Write data to a character backend from the front end.  This function will
>   * send data from the front end to the back end.  Unlike @qemu_chr_fe_write,
>   * this function will block if the back end cannot consume all of the data
>   * attempted to be written.  This function is thread-safe.
>   *
> - * @buf the data
> - * @len the number of bytes to send
> - *
>   * Returns: the number of bytes consumed (0 if no associated Chardev)
>   */
>  int qemu_chr_fe_write_all(CharBackend *be, const uint8_t *buf, int len);
[...]

With the missing colons fixed, and preferably with the commit message
clarified:
Reviewed-by: Markus Armbruster <address@hidden>