diff mbox

[v5,09/46] qapi: Use consistent generated code patterns

Message ID 1442872682-6523-10-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake Sept. 21, 2015, 9:57 p.m. UTC 
We had some pointless differences in the generated code for visit,
command marshalling, and events; unifying them makes it easier for
future patches to consolidate to common helper functions.
- Consistently name the error variable 'err'
- Consistently use the labels 'out' and 'out_obj'
- If allocation fails, jump to the right label rather than
indenting everything else

No change in semantics to the generated code.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-commands.py |  22 +++++-----
 scripts/qapi-event.py    |  26 +++++------
 scripts/qapi-visit.py    | 111 ++++++++++++++++++++++++-----------------------
 3 files changed, 80 insertions(+), 79 deletions(-)

Comments

Markus Armbruster Sept. 25, 2015, 4:54 p.m. UTC | #1 
Eric Blake <eblake@redhat.com> writes:

> We had some pointless differences in the generated code for visit,
> command marshalling, and events; unifying them makes it easier for
> future patches to consolidate to common helper functions.
> - Consistently name the error variable 'err'

Before the patch, qapi-event.py consistently uses local_err, and
qapi-commands.py uses both names.  I like your change.

> - Consistently use the labels 'out' and 'out_obj'

Before, qapi-event.py consistently uses 'clean', and qapi-visit.py uses
both 'out_obj' and 'out_end'.  Good change.

> - If allocation fails, jump to the right label rather than
> indenting everything else

Good idea.  Just one instance, in gen_visit_union().

Not mentioned: name the Visitor * parameter 'v' instead of 'm'.  No idea
where that 'm' comes from, but it has spilled over into qom/object.c and
qom/qom-object.c.  Let's ignore that for now.

> No change in semantics to the generated code.

Yes, but you need to update docs/qapi-code-gen.txt.

Pretty mechanical changes.  They look good to me, but as always, this
kind is easier to review when you do exactly one mechanical change per
patch.
Eric Blake Sept. 25, 2015, 7:06 p.m. UTC | #2 
On 09/25/2015 10:54 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> We had some pointless differences in the generated code for visit,
>> command marshalling, and events; unifying them makes it easier for
>> future patches to consolidate to common helper functions.
>> - Consistently name the error variable 'err'
> 
> Before the patch, qapi-event.py consistently uses local_err, and
> qapi-commands.py uses both names.  I like your change.
> 
>> - Consistently use the labels 'out' and 'out_obj'
> 
> Before, qapi-event.py consistently uses 'clean', and qapi-visit.py uses
> both 'out_obj' and 'out_end'.  Good change.
> 
>> - If allocation fails, jump to the right label rather than
>> indenting everything else
> 
> Good idea.  Just one instance, in gen_visit_union().
> 
> Not mentioned: name the Visitor * parameter 'v' instead of 'm'.

Whoops - I merged several patches from v4 into just this one for v5, but
forgot to update the commit message to match.

>  No idea
> where that 'm' comes from, but it has spilled over into qom/object.c and
> qom/qom-object.c.  Let's ignore that for now.

I didn't even notice that.

> 
>> No change in semantics to the generated code.
> 
> Yes, but you need to update docs/qapi-code-gen.txt.

Indeed.

> 
> Pretty mechanical changes.  They look good to me, but as always, this
> kind is easier to review when you do exactly one mechanical change per
> patch.

Sounds like my merge was premature; I'll split this back out into
separate patches, and update the docs to match (where things are
affected - our docs don't give any examples of the C code for visiting a
union).
diff mbox

Patch

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 810a897..2151120 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -53,14 +53,14 @@  def gen_call(name, arg_type, ret_type):
     push_indent()
     ret = mcgen('''

-%(lhs)sqmp_%(c_name)s(%(args)s&local_err);
+%(lhs)sqmp_%(c_name)s(%(args)s&err);
 ''',
                 c_name=c_name(name), args=argstr, lhs=lhs)
     if ret_type:
-        ret += gen_err_check('local_err')
+        ret += gen_err_check('err')
         ret += mcgen('''

-qmp_marshal_output_%(c_name)s(retval, ret, &local_err);
+qmp_marshal_output_%(c_name)s(retval, ret, &err);
 ''',
                      c_name=ret_type.c_name())
     pop_indent()
@@ -69,7 +69,7 @@  qmp_marshal_output_%(c_name)s(retval, ret, &local_err);

 def gen_marshal_vars(arg_type, ret_type):
     ret = mcgen('''
-    Error *local_err = NULL;
+    Error *err = NULL;
 ''')

     push_indent()
@@ -127,8 +127,8 @@  md = qapi_dealloc_visitor_new();
 v = qapi_dealloc_get_visitor(md);
 ''')
     else:
-        errparg = '&local_err'
-        errarg = 'local_err'
+        errparg = '&err'
+        errarg = 'err'
         ret += mcgen('''
 v = qmp_input_get_visitor(mi);
 ''')
@@ -171,20 +171,20 @@  def gen_marshal_output(ret_type):

 static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in, QObject **ret_out, Error **errp)
 {
-    Error *local_err = NULL;
+    Error *err = NULL;
     QmpOutputVisitor *mo = qmp_output_visitor_new();
     QapiDeallocVisitor *md;
     Visitor *v;

     v = qmp_output_get_visitor(mo);
-    visit_type_%(c_name)s(v, &ret_in, "unused", &local_err);
-    if (local_err) {
+    visit_type_%(c_name)s(v, &ret_in, "unused", &err);
+    if (err) {
         goto out;
     }
     *ret_out = qmp_output_get_qobject(mo);

 out:
-    error_propagate(errp, local_err);
+    error_propagate(errp, err);
     qmp_output_visitor_cleanup(mo);
     md = qapi_dealloc_visitor_new();
     v = qapi_dealloc_get_visitor(md);
@@ -227,7 +227,7 @@  def gen_marshal(name, arg_type, ret_type):
 out:
 ''')
     ret += mcgen('''
-    error_propagate(errp, local_err);
+    error_propagate(errp, err);
 ''')
     ret += gen_marshal_input_visit(arg_type, dealloc=True)
     ret += mcgen('''
diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
index d15fad9..b43bbc2 100644
--- a/scripts/qapi-event.py
+++ b/scripts/qapi-event.py
@@ -34,7 +34,7 @@  def gen_event_send(name, arg_type):
 %(proto)s
 {
     QDict *qmp;
-    Error *local_err = NULL;
+    Error *err = NULL;
     QMPEventFuncEmit emit;
 ''',
                 proto=gen_event_send_proto(name, arg_type))
@@ -67,9 +67,9 @@  def gen_event_send(name, arg_type):
     g_assert(v);

     /* Fake visit, as if all members are under a structure */
-    visit_start_struct(v, NULL, "", "%(name)s", 0, &local_err);
-    if (local_err) {
-        goto clean;
+    visit_start_struct(v, NULL, "", "%(name)s", 0, &err);
+    if (err) {
+        goto out;
     }

 ''',
@@ -90,9 +90,9 @@  def gen_event_send(name, arg_type):
                 cast = ''

             ret += mcgen('''
-    visit_type_%(c_type)s(v, %(cast)s&%(c_name)s, "%(name)s", &local_err);
-    if (local_err) {
-        goto clean;
+    visit_type_%(c_type)s(v, %(cast)s&%(c_name)s, "%(name)s", &err);
+    if (err) {
+        goto out;
     }
 ''',
                          cast=cast,
@@ -108,9 +108,9 @@  def gen_event_send(name, arg_type):

         ret += mcgen('''

-    visit_end_struct(v, &local_err);
-    if (local_err) {
-        goto clean;
+    visit_end_struct(v, &err);
+    if (err) {
+        goto out;
     }

     obj = qmp_output_get_qobject(qov);
@@ -120,18 +120,18 @@  def gen_event_send(name, arg_type):
 ''')

     ret += mcgen('''
-    emit(%(c_enum)s, qmp, &local_err);
+    emit(%(c_enum)s, qmp, &err);

 ''',
                  c_enum=c_enum_const(event_enum_name, name))

     if arg_type and arg_type.members:
         ret += mcgen('''
- clean:
+ out:
     qmp_output_visitor_cleanup(qov);
 ''')
     ret += mcgen('''
-    error_propagate(errp, local_err);
+    error_propagate(errp, err);
     QDECREF(qmp);
 }
 ''')
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index d911b20..9c0328d 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -24,7 +24,7 @@  def gen_visit_decl(name, scalar=False):
     if not scalar:
         c_type += '*'
     return mcgen('''
-void visit_type_%(c_name)s(Visitor *m, %(c_type)sobj, const char *name, Error **errp);
+void visit_type_%(c_name)s(Visitor *v, %(c_type)sobj, const char *name, Error **errp);
 ''',
                  c_name=c_name(name), c_type=c_type)

@@ -39,20 +39,20 @@  def gen_visit_implicit_struct(typ):
         # Need a forward declaration
         ret += mcgen('''

-static void visit_type_%(c_type)s_fields(Visitor *m, %(c_type)s **obj, Error **errp);
+static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s **obj, Error **errp);
 ''',
                      c_type=typ.c_name())

     ret += mcgen('''

-static void visit_type_implicit_%(c_type)s(Visitor *m, %(c_type)s **obj, Error **errp)
+static void visit_type_implicit_%(c_type)s(Visitor *v, %(c_type)s **obj, Error **errp)
 {
     Error *err = NULL;

-    visit_start_implicit_struct(m, (void **)obj, sizeof(%(c_type)s), &err);
+    visit_start_implicit_struct(v, (void **)obj, sizeof(%(c_type)s), &err);
     if (!err) {
-        visit_type_%(c_type)s_fields(m, obj, &err);
-        visit_end_implicit_struct(m, err ? NULL : &err);
+        visit_type_%(c_type)s_fields(v, obj, &err);
+        visit_end_implicit_struct(v, err ? NULL : &err);
     }
     error_propagate(errp, err);
 }
@@ -71,7 +71,7 @@  def gen_visit_struct_fields(name, base, members):

     ret += mcgen('''

-static void visit_type_%(c_name)s_fields(Visitor *m, %(c_name)s **obj, Error **errp)
+static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **errp)
 {
     Error *err = NULL;

@@ -81,7 +81,7 @@  static void visit_type_%(c_name)s_fields(Visitor *m, %(c_name)s **obj, Error **e

     if base:
         ret += mcgen('''
-visit_type_implicit_%(c_type)s(m, &(*obj)->%(c_name)s, &err);
+visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
 if (err) {
     goto out;
 }
@@ -91,14 +91,14 @@  if (err) {
     for memb in members:
         if memb.optional:
             ret += mcgen('''
-visit_optional(m, &(*obj)->has_%(c_name)s, "%(name)s", &err);
+visit_optional(v, &(*obj)->has_%(c_name)s, "%(name)s", &err);
 if (!err && (*obj)->has_%(c_name)s) {
 ''',
                          c_name=c_name(memb.name), name=memb.name)
             push_indent()

         ret += mcgen('''
-visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "%(name)s", &err);
+visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
 ''',
                      c_type=memb.type.c_name(), c_name=c_name(memb.name),
                      name=memb.name)
@@ -136,16 +136,16 @@  def gen_visit_struct(name, base, members):
     # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
     ret += mcgen('''

-void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;

-    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
+    visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
     if (!err) {
         if (*obj) {
-            visit_type_%(c_name)s_fields(m, obj, &err);
+            visit_type_%(c_name)s_fields(v, obj, &err);
         }
-        visit_end_struct(m, err ? NULL : &err);
+        visit_end_struct(v, err ? NULL : &err);
     }
     error_propagate(errp, err);
 }
@@ -158,24 +158,24 @@  void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
 def gen_visit_list(name, element_type):
     return mcgen('''

-void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;
     GenericList *i, **prev;

-    visit_start_list(m, name, &err);
+    visit_start_list(v, name, &err);
     if (err) {
         goto out;
     }

     for (prev = (GenericList **)obj;
-         !err && (i = visit_next_list(m, prev, &err)) != NULL;
+         !err && (i = visit_next_list(v, prev, &err)) != NULL;
          prev = &i) {
         %(c_name)s *native_i = (%(c_name)s *)i;
-        visit_type_%(c_elt_type)s(m, &native_i->value, NULL, &err);
+        visit_type_%(c_elt_type)s(v, &native_i->value, NULL, &err);
     }

-    visit_end_list(m, err ? NULL : &err);
+    visit_end_list(v, err ? NULL : &err);
 out:
     error_propagate(errp, err);
 }
@@ -186,9 +186,9 @@  out:
 def gen_visit_enum(name):
     return mcgen('''

-void visit_type_%(c_name)s(Visitor *m, %(c_name)s *obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s *obj, const char *name, Error **errp)
 {
-    visit_type_enum(m, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp);
+    visit_type_enum(v, (int *)obj, %(c_name)s_lookup, "%(name)s", name, errp);
 }
 ''',
                  c_name=c_name(name), name=name)
@@ -197,17 +197,17 @@  void visit_type_%(c_name)s(Visitor *m, %(c_name)s *obj, const char *name, Error
 def gen_visit_alternate(name, variants):
     ret = mcgen('''

-void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;

-    visit_start_implicit_struct(m, (void**) obj, sizeof(%(c_name)s), &err);
+    visit_start_implicit_struct(v, (void**) obj, sizeof(%(c_name)s), &err);
     if (err) {
         goto out;
     }
-    visit_get_next_type(m, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, &err);
+    visit_get_next_type(v, (int*) &(*obj)->kind, %(c_name)s_qtypes, name, &err);
     if (err) {
-        goto out_end;
+        goto out_obj;
     }
     switch ((*obj)->kind) {
 ''',
@@ -216,7 +216,7 @@  void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
     for var in variants.variants:
         ret += mcgen('''
     case %(case)s:
-        visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, name, &err);
+        visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, name, &err);
         break;
 ''',
                      case=c_enum_const(variants.tag_member.type.name,
@@ -228,8 +228,8 @@  void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
     default:
         abort();
     }
-out_end:
-    visit_end_implicit_struct(m, err ? NULL : &err);
+out_obj:
+    visit_end_implicit_struct(v, err ? NULL : &err);
 out:
     error_propagate(errp, err);
 }
@@ -252,24 +252,26 @@  def gen_visit_union(name, base, variants):

     ret += mcgen('''

-void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error **errp)
+void visit_type_%(c_name)s(Visitor *v, %(c_name)s **obj, const char *name, Error **errp)
 {
     Error *err = NULL;

-    visit_start_struct(m, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
+    visit_start_struct(v, (void **)obj, "%(name)s", name, sizeof(%(c_name)s), &err);
     if (err) {
         goto out;
     }
-    if (*obj) {
+    if (!*obj) {
+        goto out_obj;
+    }
 ''',
                  c_name=c_name(name), name=name)

     if base:
         ret += mcgen('''
-        visit_type_%(c_name)s_fields(m, obj, &err);
-        if (err) {
-            goto out_obj;
-        }
+    visit_type_%(c_name)s_fields(v, obj, &err);
+    if (err) {
+        goto out_obj;
+    }
 ''',
                      c_name=c_name(name))

@@ -278,14 +280,14 @@  void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
         # we pointlessly use a different key for simple unions
         tag_key = 'type'
     ret += mcgen('''
-        visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "%(name)s", &err);
-        if (err) {
-            goto out_obj;
-        }
-        if (!visit_start_union(m, !!(*obj)->data, &err) || err) {
-            goto out_obj;
-        }
-        switch ((*obj)->%(c_name)s) {
+    visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
+    if (err) {
+        goto out_obj;
+    }
+    if (!visit_start_union(v, !!(*obj)->data, &err) || err) {
+        goto out_obj;
+    }
+    switch ((*obj)->%(c_name)s) {
 ''',
                  c_type=variants.tag_member.type.c_name(),
                  # TODO ugly special case for simple union
@@ -298,36 +300,35 @@  void visit_type_%(c_name)s(Visitor *m, %(c_name)s **obj, const char *name, Error
         # TODO ugly special case for simple union
         simple_union_type = var.simple_union_type()
         ret += mcgen('''
-        case %(case)s:
+    case %(case)s:
 ''',
                      case=c_enum_const(variants.tag_member.type.name,
                                        var.name))
         if simple_union_type:
             ret += mcgen('''
-            visit_type_%(c_type)s(m, &(*obj)->%(c_name)s, "data", &err);
+        visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "data", &err);
 ''',
                          c_type=simple_union_type.c_name(),
                          c_name=c_name(var.name))
         else:
             ret += mcgen('''
-            visit_type_implicit_%(c_type)s(m, &(*obj)->%(c_name)s, &err);
+        visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
 ''',
                          c_type=var.type.c_name(),
                          c_name=c_name(var.name))
         ret += mcgen('''
-            break;
+        break;
 ''')

     ret += mcgen('''
-        default:
-            abort();
-        }
-out_obj:
-        error_propagate(errp, err);
-        err = NULL;
-        visit_end_union(m, !!(*obj)->data, &err);
+    default:
+        abort();
     }
-    visit_end_struct(m, err ? NULL : &err);
+out_obj:
+    error_propagate(errp, err);
+    err = NULL;
+    visit_end_union(v, !!(*obj)->data, &err);
+    visit_end_struct(v, err ? NULL : &err);
 out:
     error_propagate(errp, err);
 }