Re: [Qemu-devel] [PATCH v3 06/18] qapi: Add qstring_append_format()

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Back in commit 764c1ca (Nov 2009), we added qstring_append_int().
> However, it did not see any use until commit 190c882 (Jan 2015).
> Furthermore, it has a rather limited use case - to print anything
> else, callers still have to format into a temporary buffer, unless
> we want to introduce an explosion of new qstring_append_* methods
> for each useful type to print.
>
> A much better approach is to add a wrapper that merges printf
> behavior onto qstring_append, via the new qstring_append_format()
> (and its vararg counterpart).  In fact, with that in place, we
> no longer need qstring_append_int().
>
> Other immediate uses for the new function include simplifying
> two existing clients of qstring_append() on a just-formatted
> buffer, and the fact that we can take advantage of printf width
> manipulations for more efficient indentation.
>
> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: Fam Zheng <address@hidden>
>
> ---
> v3: rebase to master
> v2: also simplify qstring_append_json_string(), add assertion that
> format is well-formed
> ---
>  include/qapi/qmp/qstring.h |  7 +++++--
>  qjson.c                    |  2 +-
>  qobject/qobject-json.c     | 25 +++++--------------------
>  qobject/qstring.c          | 37 +++++++++++++++++++++++++------------
>  4 files changed, 36 insertions(+), 35 deletions(-)
>
> diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
> index f00e3df..10afa5d 100644
> --- a/include/qapi/qmp/qstring.h
> +++ b/include/qapi/qmp/qstring.h
> @@ -1,7 +1,7 @@
>  /*
>   * QString Module
>   *
> - * Copyright (C) 2009, 2015 Red Hat Inc.
> + * Copyright (C) 2009-2016 Red Hat Inc.
>   *
>   * Authors:
>   *  Luiz Capitulino <address@hidden>
> @@ -28,9 +28,12 @@ QString *qstring_from_str(const char *str);
>  QString *qstring_from_substr(const char *str, int start, int end);
>  size_t qstring_get_length(const QString *qstring);
>  const char *qstring_get_str(const QString *qstring);
> -void qstring_append_int(QString *qstring, int64_t value);
>  void qstring_append(QString *qstring, const char *str);
>  void qstring_append_chr(QString *qstring, int c);
> +void qstring_append_format(QString *qstring, const char *fmt, ...)
> +    GCC_FMT_ATTR(2, 3);
> +void qstring_append_vformat(QString *qstring, const char *fmt, va_list ap)
> +    GCC_FMT_ATTR(2, 0);

Let's call these qstring_append_printf() and qstring_append_vprintf(),
to match GLib's g_string_append_printf() and g_string_append_vprintf().

>  void qstring_append_json_string(QString *qstring, const char *raw);
>  void qstring_append_json_number(QString *qstring, double number, Error 
> **errp);
>  QString *qobject_to_qstring(const QObject *obj);
> diff --git a/qjson.c b/qjson.c
> index b9a9a36..d172b1f 100644
> --- a/qjson.c
> +++ b/qjson.c
> @@ -70,7 +70,7 @@ void json_end_array(QJSON *json)
>  void json_prop_int(QJSON *json, const char *name, int64_t val)
>  {
>      json_emit_element(json, name);
> -    qstring_append_int(json->str, val);
> +    qstring_append_format(json->str, "%" PRId64, val);
>  }
>
>  void json_prop_str(QJSON *json, const char *name, const char *str)
> diff --git a/qobject/qobject-json.c b/qobject/qobject-json.c
> index 97bccb7..963de07 100644
> --- a/qobject/qobject-json.c
> +++ b/qobject/qobject-json.c
> @@ -80,16 +80,13 @@ static void to_json(const QObject *obj, QString *str, int 
> pretty, int indent);
>  static void to_json_dict_iter(const char *key, QObject *obj, void *opaque)
>  {
>      ToJsonIterState *s = opaque;
> -    int j;
>
>      if (s->count) {
>          qstring_append(s->str, s->pretty ? "," : ", ");
>      }
>
>      if (s->pretty) {
> -        qstring_append(s->str, "\n");
> -        for (j = 0 ; j < s->indent ; j++)
> -            qstring_append(s->str, "    ");
> +        qstring_append_format(s->str, "\n%*s", 4 * s->indent, "");
>      }
>
>      qstring_append_json_string(s->str, key);
> @@ -102,16 +99,13 @@ static void to_json_dict_iter(const char *key, QObject 
> *obj, void *opaque)
>  static void to_json_list_iter(QObject *obj, void *opaque)
>  {
>      ToJsonIterState *s = opaque;
> -    int j;
>
>      if (s->count) {
>          qstring_append(s->str, s->pretty ? "," : ", ");
>      }
>
>      if (s->pretty) {
> -        qstring_append(s->str, "\n");
> -        for (j = 0 ; j < s->indent ; j++)
> -            qstring_append(s->str, "    ");
> +        qstring_append_format(s->str, "\n%*s", 4 * s->indent, "");
>      }
>
>      to_json(obj, s->str, s->pretty, s->indent);
> @@ -126,10 +120,7 @@ static void to_json(const QObject *obj, QString *str, 
> int pretty, int indent)
>          break;
>      case QTYPE_QINT: {
>          QInt *val = qobject_to_qint(obj);
> -        char buffer[1024];
> -
> -        snprintf(buffer, sizeof(buffer), "%" PRId64, qint_get_int(val));
> -        qstring_append(str, buffer);
> +        qstring_append_format(str, "%" PRId64, qint_get_int(val));
>          break;
>      }
>      case QTYPE_QSTRING: {
> @@ -148,10 +139,7 @@ static void to_json(const QObject *obj, QString *str, 
> int pretty, int indent)
>          qstring_append(str, "{");
>          qdict_iter(val, to_json_dict_iter, &s);
>          if (pretty) {
> -            int j;
> -            qstring_append(str, "\n");
> -            for (j = 0 ; j < indent ; j++)
> -                qstring_append(str, "    ");
> +            qstring_append_format(str, "\n%*s", 4 * indent, "");
>          }
>          qstring_append(str, "}");
>          break;
> @@ -167,10 +155,7 @@ static void to_json(const QObject *obj, QString *str, 
> int pretty, int indent)
>          qstring_append(str, "[");
>          qlist_iter(val, (void *)to_json_list_iter, &s);
>          if (pretty) {
> -            int j;
> -            qstring_append(str, "\n");
> -            for (j = 0 ; j < indent ; j++)
> -                qstring_append(str, "    ");
> +            qstring_append_format(str, "\n%*s", 4 * indent, "");
>          }
>          qstring_append(str, "]");
>          break;
> diff --git a/qobject/qstring.c b/qobject/qstring.c
> index 618dd8f..0285d53 100644
> --- a/qobject/qstring.c
> +++ b/qobject/qstring.c
> @@ -90,12 +90,28 @@ void qstring_append(QString *qstring, const char *str)
>      qstring->string[qstring->length] = 0;
>  }
>
> -void qstring_append_int(QString *qstring, int64_t value)
> +void qstring_append_format(QString *qstring, const char *fmt, ...)
>  {
> -    char num[32];
> +    va_list ap;
>
> -    snprintf(num, sizeof(num), "%" PRId64, value);
> -    qstring_append(qstring, num);
> +    va_start(ap, fmt);
> +    qstring_append_vformat(qstring, fmt, ap);
> +    va_end(ap);
> +}
> +
> +void qstring_append_vformat(QString *qstring, const char *fmt, va_list ap)
> +{
> +    va_list ap2;
> +    int len;
> +
> +    va_copy(ap2, ap);
> +    len = vsnprintf(qstring->string + qstring->length, 0, fmt, ap2);

Why bother with the first argument here?  NULL would do :)

> +    assert(len >= 0);
> +    va_end(ap2);
> +
> +    capacity_increase(qstring, len);
> +    vsnprintf(qstring->string + qstring->length, len + 1, fmt, ap);
> +    qstring->length += len;
>  }
>
>  /**
> @@ -116,7 +132,6 @@ void qstring_append_json_string(QString *qstring, const 
> char *raw)
>  {
>      const char *ptr = raw;
>      int cp;
> -    char buf[16];
>      char *end;
>
>      qstring_append(qstring, "\"");
> @@ -151,16 +166,14 @@ void qstring_append_json_string(QString *qstring, const 
> char *raw)
>              }
>              if (cp > 0xFFFF) {
>                  /* beyond BMP; need a surrogate pair */
> -                snprintf(buf, sizeof(buf), "\\u%04X\\u%04X",
> -                         0xD800 + ((cp - 0x10000) >> 10),
> -                         0xDC00 + ((cp - 0x10000) & 0x3FF));
> +                qstring_append_format(qstring, "\\u%04X\\u%04X",
> +                                      0xD800 + ((cp - 0x10000) >> 10),
> +                                      0xDC00 + ((cp - 0x10000) & 0x3FF));
>              } else if (cp < 0x20 || cp >= 0x7F) {
> -                snprintf(buf, sizeof(buf), "\\u%04X", cp);
> +                qstring_append_format(qstring, "\\u%04X", cp);
>              } else {
> -                buf[0] = cp;
> -                buf[1] = 0;
> +                qstring_append_chr(qstring, cp);
>              }
> -            qstring_append(qstring, buf);
>          }
>      };