diff mbox

[v9,02/37] qapi: Avoid use of misnamed DO_UPCAST()

Message ID 1453219845-30939-3-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake Jan. 19, 2016, 4:10 p.m. UTC 
The macro DO_UPCAST() is incorrectly named: it converts from a
parent class to a derived class (which is a downcast).  Better,
and more consistent with some of the other qapi visitors, is
to use the container_of() macro through a to_FOO() helper.

Our current definition of container_of() is weaker than
DO_UPCAST(), in that it does not require the derived class to
have Visitor as its first member, but this does not hurt our
usage patterns in qapi visitors.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
v9: no change
v8: no change
v7: new patch
---
 qapi/opts-visitor.c          | 28 +++++++++++++++++-----------
 qapi/string-input-visitor.c  | 23 ++++++++++++++---------
 qapi/string-output-visitor.c | 21 +++++++++++++--------
 3 files changed, 44 insertions(+), 28 deletions(-)

Comments

Markus Armbruster Jan. 20, 2016, 10:04 a.m. UTC | #1 
Eric Blake <eblake@redhat.com> writes:

> The macro DO_UPCAST() is incorrectly named: it converts from a
> parent class to a derived class (which is a downcast).  Better,
> and more consistent with some of the other qapi visitors, is
> to use the container_of() macro through a to_FOO() helper.
>
> Our current definition of container_of() is weaker than
> DO_UPCAST(), in that it does not require the derived class to
> have Visitor as its first member, but this does not hurt our
> usage patterns in qapi visitors.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> ---
> v9: no change
> v8: no change
> v7: new patch
> ---
>  qapi/opts-visitor.c          | 28 +++++++++++++++++-----------
>  qapi/string-input-visitor.c  | 23 ++++++++++++++---------
>  qapi/string-output-visitor.c | 21 +++++++++++++--------
>  3 files changed, 44 insertions(+), 28 deletions(-)
>
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index ef5fb8b..dd4094c 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -89,6 +89,12 @@ struct OptsVisitor
>  };
>
>
> +static OptsVisitor *to_ov(Visitor *v)
> +{
> +    return container_of(v, OptsVisitor, visitor);
> +}
> +
> +

The name to_ov() is rather laconic even for my taste.  Tolerable, since
it's static.

>  static void
>  destroy_list(gpointer list)
>  {
> @@ -121,7 +127,7 @@ static void
>  opts_start_struct(Visitor *v, void **obj, const char *kind,
>                    const char *name, size_t size, Error **errp)
>  {
> -    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
> +    OptsVisitor *ov = to_ov(v);
>      const QemuOpt *opt;
>
>      if (obj) {

Much clearer!

[...]

Patch gets rid of all DO_UPCAST() in qapi/.  PATCH 33 brings one back;
it should use to_ov() instead.
Eric Blake Jan. 20, 2016, 3:59 p.m. UTC | #2 
On 01/20/2016 03:04 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> The macro DO_UPCAST() is incorrectly named: it converts from a
>> parent class to a derived class (which is a downcast).  Better,
>> and more consistent with some of the other qapi visitors, is
>> to use the container_of() macro through a to_FOO() helper.
>>
>> Our current definition of container_of() is weaker than
>> DO_UPCAST(), in that it does not require the derived class to
>> have Visitor as its first member, but this does not hurt our
>> usage patterns in qapi visitors.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>

>>
>> +static OptsVisitor *to_ov(Visitor *v)
>> +{
>> +    return container_of(v, OptsVisitor, visitor);
>> +}
>> +
>> +
> 
> The name to_ov() is rather laconic even for my taste.  Tolerable, since
> it's static.

And matches existing practice pre-patch:

qapi/qapi-dealloc-visitor.c:static QapiDeallocVisitor *to_qov(Visitor *v)
qapi/qmp-input-visitor.c:static QmpInputVisitor *to_qiv(Visitor *v)
qapi/qmp-output-visitor.c:static QmpOutputVisitor *to_qov(Visitor *v)


> 
> Patch gets rid of all DO_UPCAST() in qapi/.  PATCH 33 brings one back;
> it should use to_ov() instead.

D'oh. You can tell what order I wrote the patches in :)

If I need to respin, I'll fix it; otherwise, I assume you can handle it.
Markus Armbruster Jan. 21, 2016, 6:22 a.m. UTC | #3 
Eric Blake <eblake@redhat.com> writes:

> On 01/20/2016 03:04 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> The macro DO_UPCAST() is incorrectly named: it converts from a
>>> parent class to a derived class (which is a downcast).  Better,
>>> and more consistent with some of the other qapi visitors, is
>>> to use the container_of() macro through a to_FOO() helper.
>>>
>>> Our current definition of container_of() is weaker than
>>> DO_UPCAST(), in that it does not require the derived class to
>>> have Visitor as its first member, but this does not hurt our
>>> usage patterns in qapi visitors.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>
>>>
>>> +static OptsVisitor *to_ov(Visitor *v)
>>> +{
>>> +    return container_of(v, OptsVisitor, visitor);
>>> +}
>>> +
>>> +
>> 
>> The name to_ov() is rather laconic even for my taste.  Tolerable, since
>> it's static.
>
> And matches existing practice pre-patch:
>
> qapi/qapi-dealloc-visitor.c:static QapiDeallocVisitor *to_qov(Visitor *v)
> qapi/qmp-input-visitor.c:static QmpInputVisitor *to_qiv(Visitor *v)
> qapi/qmp-output-visitor.c:static QmpOutputVisitor *to_qov(Visitor *v)
>
>
>> 
>> Patch gets rid of all DO_UPCAST() in qapi/.  PATCH 33 brings one back;
>> it should use to_ov() instead.
>
> D'oh. You can tell what order I wrote the patches in :)
>
> If I need to respin, I'll fix it; otherwise, I assume you can handle it.

Yes.
diff mbox

Patch

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index ef5fb8b..dd4094c 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -89,6 +89,12 @@  struct OptsVisitor
 };


+static OptsVisitor *to_ov(Visitor *v)
+{
+    return container_of(v, OptsVisitor, visitor);
+}
+
+
 static void
 destroy_list(gpointer list)
 {
@@ -121,7 +127,7 @@  static void
 opts_start_struct(Visitor *v, void **obj, const char *kind,
                   const char *name, size_t size, Error **errp)
 {
-    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+    OptsVisitor *ov = to_ov(v);
     const QemuOpt *opt;

     if (obj) {
@@ -160,7 +166,7 @@  ghr_true(gpointer ign_key, gpointer ign_value, gpointer ign_user_data)
 static void
 opts_end_struct(Visitor *v, Error **errp)
 {
-    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+    OptsVisitor *ov = to_ov(v);
     GQueue *any;

     if (--ov->depth > 0) {
@@ -202,7 +208,7 @@  lookup_distinct(const OptsVisitor *ov, const char *name, Error **errp)
 static void
 opts_start_list(Visitor *v, const char *name, Error **errp)
 {
-    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+    OptsVisitor *ov = to_ov(v);

     /* we can't traverse a list in a list */
     assert(ov->list_mode == LM_NONE);
@@ -216,7 +222,7 @@  opts_start_list(Visitor *v, const char *name, Error **errp)
 static GenericList *
 opts_next_list(Visitor *v, GenericList **list, Error **errp)
 {
-    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+    OptsVisitor *ov = to_ov(v);
     GenericList **link;

     switch (ov->list_mode) {
@@ -265,7 +271,7 @@  opts_next_list(Visitor *v, GenericList **list, Error **errp)
 static void
 opts_end_list(Visitor *v, Error **errp)
 {
-    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+    OptsVisitor *ov = to_ov(v);

     assert(ov->list_mode == LM_STARTED ||
            ov->list_mode == LM_IN_PROGRESS ||
@@ -307,7 +313,7 @@  processed(OptsVisitor *ov, const char *name)
 static void
 opts_type_str(Visitor *v, char **obj, const char *name, Error **errp)
 {
-    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+    OptsVisitor *ov = to_ov(v);
     const QemuOpt *opt;

     opt = lookup_scalar(ov, name, errp);
@@ -323,7 +329,7 @@  opts_type_str(Visitor *v, char **obj, const char *name, Error **errp)
 static void
 opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
 {
-    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+    OptsVisitor *ov = to_ov(v);
     const QemuOpt *opt;

     opt = lookup_scalar(ov, name, errp);
@@ -356,7 +362,7 @@  opts_type_bool(Visitor *v, bool *obj, const char *name, Error **errp)
 static void
 opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
 {
-    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+    OptsVisitor *ov = to_ov(v);
     const QemuOpt *opt;
     const char *str;
     long long val;
@@ -412,7 +418,7 @@  opts_type_int(Visitor *v, int64_t *obj, const char *name, Error **errp)
 static void
 opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
 {
-    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+    OptsVisitor *ov = to_ov(v);
     const QemuOpt *opt;
     const char *str;
     unsigned long long val;
@@ -464,7 +470,7 @@  opts_type_uint64(Visitor *v, uint64_t *obj, const char *name, Error **errp)
 static void
 opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
 {
-    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+    OptsVisitor *ov = to_ov(v);
     const QemuOpt *opt;
     int64_t val;
     char *endptr;
@@ -490,7 +496,7 @@  opts_type_size(Visitor *v, uint64_t *obj, const char *name, Error **errp)
 static void
 opts_optional(Visitor *v, bool *present, const char *name)
 {
-    OptsVisitor *ov = DO_UPCAST(OptsVisitor, visitor, v);
+    OptsVisitor *ov = to_ov(v);

     /* we only support a single mandatory scalar field in a list node */
     assert(ov->list_mode == LM_NONE);
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index dee780a..7f5645b 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -32,6 +32,11 @@  struct StringInputVisitor
     const char *string;
 };

+static StringInputVisitor *to_siv(Visitor *v)
+{
+    return container_of(v, StringInputVisitor, visitor);
+}
+
 static void free_range(void *range, void *dummy)
 {
     g_free(range);
@@ -120,7 +125,7 @@  error:
 static void
 start_list(Visitor *v, const char *name, Error **errp)
 {
-    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    StringInputVisitor *siv = to_siv(v);

     parse_str(siv, errp);

@@ -136,7 +141,7 @@  start_list(Visitor *v, const char *name, Error **errp)
 static GenericList *
 next_list(Visitor *v, GenericList **list, Error **errp)
 {
-    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    StringInputVisitor *siv = to_siv(v);
     GenericList **link;
     Range *r;

@@ -175,14 +180,14 @@  next_list(Visitor *v, GenericList **list, Error **errp)
 static void
 end_list(Visitor *v, Error **errp)
 {
-    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    StringInputVisitor *siv = to_siv(v);
     siv->head = true;
 }

 static void parse_type_int(Visitor *v, int64_t *obj, const char *name,
                            Error **errp)
 {
-    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    StringInputVisitor *siv = to_siv(v);

     if (!siv->string) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -224,7 +229,7 @@  error:
 static void parse_type_size(Visitor *v, uint64_t *obj, const char *name,
                             Error **errp)
 {
-    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    StringInputVisitor *siv = to_siv(v);
     Error *err = NULL;
     uint64_t val;

@@ -246,7 +251,7 @@  static void parse_type_size(Visitor *v, uint64_t *obj, const char *name,
 static void parse_type_bool(Visitor *v, bool *obj, const char *name,
                             Error **errp)
 {
-    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    StringInputVisitor *siv = to_siv(v);

     if (siv->string) {
         if (!strcasecmp(siv->string, "on") ||
@@ -270,7 +275,7 @@  static void parse_type_bool(Visitor *v, bool *obj, const char *name,
 static void parse_type_str(Visitor *v, char **obj, const char *name,
                            Error **errp)
 {
-    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    StringInputVisitor *siv = to_siv(v);
     if (siv->string) {
         *obj = g_strdup(siv->string);
     } else {
@@ -282,7 +287,7 @@  static void parse_type_str(Visitor *v, char **obj, const char *name,
 static void parse_type_number(Visitor *v, double *obj, const char *name,
                               Error **errp)
 {
-    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    StringInputVisitor *siv = to_siv(v);
     char *endp = (char *) siv->string;
     double val;

@@ -301,7 +306,7 @@  static void parse_type_number(Visitor *v, double *obj, const char *name,

 static void parse_optional(Visitor *v, bool *present, const char *name)
 {
-    StringInputVisitor *siv = DO_UPCAST(StringInputVisitor, visitor, v);
+    StringInputVisitor *siv = to_siv(v);

     if (!siv->string) {
         *present = false;
diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index b86ce2c..202764c 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -66,6 +66,11 @@  struct StringOutputVisitor
     GList *ranges;
 };

+static StringOutputVisitor *to_sov(Visitor *v)
+{
+    return container_of(v, StringOutputVisitor, visitor);
+}
+
 static void string_output_set(StringOutputVisitor *sov, char *string)
 {
     if (sov->string) {
@@ -119,7 +124,7 @@  static void format_string(StringOutputVisitor *sov, Range *r, bool next,
 static void print_type_int(Visitor *v, int64_t *obj, const char *name,
                            Error **errp)
 {
-    StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
+    StringOutputVisitor *sov = to_sov(v);
     GList *l;

     switch (sov->list_mode) {
@@ -195,7 +200,7 @@  static void print_type_int(Visitor *v, int64_t *obj, const char *name,
 static void print_type_size(Visitor *v, uint64_t *obj, const char *name,
                            Error **errp)
 {
-    StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
+    StringOutputVisitor *sov = to_sov(v);
     static const char suffixes[] = { 'B', 'K', 'M', 'G', 'T', 'P', 'E' };
     uint64_t div, val;
     char *out;
@@ -226,14 +231,14 @@  static void print_type_size(Visitor *v, uint64_t *obj, const char *name,
 static void print_type_bool(Visitor *v, bool *obj, const char *name,
                             Error **errp)
 {
-    StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
+    StringOutputVisitor *sov = to_sov(v);
     string_output_set(sov, g_strdup(*obj ? "true" : "false"));
 }

 static void print_type_str(Visitor *v, char **obj, const char *name,
                            Error **errp)
 {
-    StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
+    StringOutputVisitor *sov = to_sov(v);
     char *out;

     if (sov->human) {
@@ -247,14 +252,14 @@  static void print_type_str(Visitor *v, char **obj, const char *name,
 static void print_type_number(Visitor *v, double *obj, const char *name,
                               Error **errp)
 {
-    StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
+    StringOutputVisitor *sov = to_sov(v);
     string_output_set(sov, g_strdup_printf("%f", *obj));
 }

 static void
 start_list(Visitor *v, const char *name, Error **errp)
 {
-    StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
+    StringOutputVisitor *sov = to_sov(v);

     /* we can't traverse a list in a list */
     assert(sov->list_mode == LM_NONE);
@@ -265,7 +270,7 @@  start_list(Visitor *v, const char *name, Error **errp)
 static GenericList *
 next_list(Visitor *v, GenericList **list, Error **errp)
 {
-    StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
+    StringOutputVisitor *sov = to_sov(v);
     GenericList *ret = NULL;
     if (*list) {
         if (sov->head) {
@@ -292,7 +297,7 @@  next_list(Visitor *v, GenericList **list, Error **errp)
 static void
 end_list(Visitor *v, Error **errp)
 {
-    StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
+    StringOutputVisitor *sov = to_sov(v);

     assert(sov->list_mode == LM_STARTED ||
            sov->list_mode == LM_END ||