diff mbox

[v6,14/16] qapi: Consistent generated code: minimize push_indent() usage

Message ID 1443497249-15361-15-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake Sept. 29, 2015, 3:27 a.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.

This patch makes no difference to the generated code, but rather
focuses on reducing the number of push_indent()/pop_indent() pairs
so that generated code is typically already at its natural output
indentation in the python files.  It is easier to reason about
generated code if the reader does not have to track how much
spacing will be inserted alongside the code, and moreso when all
of the generators use the same patterns (qapi-type and qapi-event
were already using in-place indentation).

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v6: new patch, but based on theme of 9/46 and code sharing of 10/46
---
 scripts/qapi-commands.py | 54 ++++++++++++++++++++----------------------------
 scripts/qapi-visit.py    | 24 ++++++++++-----------
 2 files changed, 33 insertions(+), 45 deletions(-)

Comments

Markus Armbruster Sept. 29, 2015, 2:10 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.
>
> This patch makes no difference to the generated code, but rather
> focuses on reducing the number of push_indent()/pop_indent() pairs
> so that generated code is typically already at its natural output
> indentation in the python files.  It is easier to reason about
> generated code if the reader does not have to track how much
> spacing will be inserted alongside the code, and moreso when all
> of the generators use the same patterns (qapi-type and qapi-event
> were already using in-place indentation).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

A few places become slightly less readable because the generated code is
now indented lile the surrounding Python code.  Other places become more
readable.  Having to think less about push_indent() / pop_indent() tips
the balance, I guess.

Have you diffed the generated code before and after the patch?
Eric Blake Sept. 29, 2015, 3:07 p.m. UTC | #2
On 09/29/2015 08:10 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.
>>
>> This patch makes no difference to the generated code, but rather
>> focuses on reducing the number of push_indent()/pop_indent() pairs
>> so that generated code is typically already at its natural output
>> indentation in the python files.  It is easier to reason about
>> generated code if the reader does not have to track how much
>> spacing will be inserted alongside the code, and moreso when all
>> of the generators use the same patterns (qapi-type and qapi-event
>> were already using in-place indentation).
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> A few places become slightly less readable because the generated code is
> now indented lile the surrounding Python code.  Other places become more
> readable.  Having to think less about push_indent() / pop_indent() tips
> the balance, I guess.
> 
> Have you diffed the generated code before and after the patch?

Yes; quoting above:

>> This patch makes no difference to the generated code,
Markus Armbruster Oct. 1, 2015, 6:26 a.m. UTC | #3
Eric Blake <eblake@redhat.com> writes:

> On 09/29/2015 08:10 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.
>>>
>>> This patch makes no difference to the generated code, but rather
>>> focuses on reducing the number of push_indent()/pop_indent() pairs
>>> so that generated code is typically already at its natural output
>>> indentation in the python files.  It is easier to reason about
>>> generated code if the reader does not have to track how much
>>> spacing will be inserted alongside the code, and moreso when all
>>> of the generators use the same patterns (qapi-type and qapi-event
>>> were already using in-place indentation).
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> 
>> A few places become slightly less readable because the generated code is
>> now indented lile the surrounding Python code.  Other places become more
>> readable.  Having to think less about push_indent() / pop_indent() tips
>> the balance, I guess.
>> 
>> Have you diffed the generated code before and after the patch?
>
> Yes; quoting above:
>
>>> This patch makes no difference to the generated code,

Missed it, my apologies.
diff mbox

Patch

diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 8fab1ce..1d3c0d5 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -29,9 +29,9 @@  def gen_err_check(err):
     if not err:
         return ''
     return mcgen('''
-if (%(err)s) {
-    goto out;
-}
+    if (%(err)s) {
+        goto out;
+    }
 ''',
                  err=err)

@@ -50,20 +50,18 @@  def gen_call(name, arg_type, ret_type):
     if ret_type:
         lhs = 'retval = '

-    push_indent()
     ret = mcgen('''

-%(lhs)sqmp_%(c_name)s(%(args)s&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('err')
         ret += mcgen('''

-qmp_marshal_output_%(c_name)s(retval, ret, &err);
+    qmp_marshal_output_%(c_name)s(retval, ret, &err);
 ''',
                      c_name=ret_type.c_name())
-    pop_indent()
     return ret


@@ -72,29 +70,27 @@  def gen_marshal_vars(arg_type, ret_type):
     Error *err = NULL;
 ''')

-    push_indent()
-
     if ret_type:
         ret += mcgen('''
-%(c_type)s retval;
+    %(c_type)s retval;
 ''',
                      c_type=ret_type.c_type())

     if arg_type:
         ret += mcgen('''
-QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
-QapiDeallocVisitor *qdv;
-Visitor *v;
+    QmpInputVisitor *qiv = qmp_input_visitor_new_strict(QOBJECT(args));
+    QapiDeallocVisitor *qdv;
+    Visitor *v;
 ''')

         for memb in arg_type.members:
             if memb.optional:
                 ret += mcgen('''
-bool has_%(c_name)s = false;
+    bool has_%(c_name)s = false;
 ''',
                              c_name=c_name(memb.name))
             ret += mcgen('''
-%(c_type)s %(c_name)s = %(c_null)s;
+    %(c_type)s %(c_name)s = %(c_null)s;
 ''',
                          c_name=c_name(memb.name),
                          c_type=memb.type.c_type(),
@@ -103,10 +99,9 @@  bool has_%(c_name)s = false;
     else:
         ret += mcgen('''

-(void)args;
+    (void)args;
 ''')

-    pop_indent()
     return ret


@@ -116,38 +111,36 @@  def gen_marshal_input_visit(arg_type, dealloc=False):
     if not arg_type:
         return ret

-    push_indent()
-
     if dealloc:
         errparg = 'NULL'
         errarg = None
         ret += mcgen('''
-qmp_input_visitor_cleanup(qiv);
-qdv = qapi_dealloc_visitor_new();
-v = qapi_dealloc_get_visitor(qdv);
+    qmp_input_visitor_cleanup(qiv);
+    qdv = qapi_dealloc_visitor_new();
+    v = qapi_dealloc_get_visitor(qdv);
 ''')
     else:
         errparg = '&err'
         errarg = 'err'
         ret += mcgen('''
-v = qmp_input_get_visitor(qiv);
+    v = qmp_input_get_visitor(qiv);
 ''')

     for memb in arg_type.members:
         if memb.optional:
             ret += mcgen('''
-visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
+    visit_optional(v, &has_%(c_name)s, "%(name)s", %(errp)s);
 ''',
                          c_name=c_name(memb.name), name=memb.name,
                          errp=errparg)
             ret += gen_err_check(errarg)
             ret += mcgen('''
-if (has_%(c_name)s) {
+    if (has_%(c_name)s) {
 ''',
                          c_name=c_name(memb.name))
             push_indent()
         ret += mcgen('''
-visit_type_%(c_type)s(v, &%(c_name)s, "%(name)s", %(errp)s);
+    visit_type_%(c_type)s(v, &%(c_name)s, "%(name)s", %(errp)s);
 ''',
                      c_name=c_name(memb.name), name=memb.name,
                      c_type=memb.type.c_name(), errp=errparg)
@@ -155,14 +148,13 @@  visit_type_%(c_type)s(v, &%(c_name)s, "%(name)s", %(errp)s);
         if memb.optional:
             pop_indent()
             ret += mcgen('''
-}
+    }
 ''')

     if dealloc:
         ret += mcgen('''
-qapi_dealloc_visitor_cleanup(qdv);
+    qapi_dealloc_visitor_cleanup(qdv);
 ''')
-    pop_indent()
     return ret


@@ -237,17 +229,15 @@  def gen_marshal(name, arg_type, ret_type):


 def gen_register_command(name, success_response):
-    push_indent()
     options = 'QCO_NO_OPTIONS'
     if not success_response:
         options = 'QCO_NO_SUCCESS_RESP'

     ret = mcgen('''
-qmp_register_command("%(name)s", qmp_marshal_%(c_name)s, %(opts)s);
+    qmp_register_command("%(name)s", qmp_marshal_%(c_name)s, %(opts)s);
 ''',
                 name=name, c_name=c_name(name),
                 opts=options)
-    pop_indent()
     return ret


diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 3b1e7b0..7ce8616 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -77,28 +77,27 @@  static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s **obj, Error **e

 ''',
                  c_name=c_name(name))
-    push_indent()

     if base:
         ret += mcgen('''
-visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
-if (err) {
-    goto out;
-}
+    visit_type_implicit_%(c_type)s(v, &(*obj)->%(c_name)s, &err);
+    if (err) {
+        goto out;
+    }
 ''',
                      c_type=base.c_name(), c_name=c_name('base'))

     for memb in members:
         if memb.optional:
             ret += mcgen('''
-visit_optional(v, &(*obj)->has_%(c_name)s, "%(name)s", &err);
-if (!err && (*obj)->has_%(c_name)s) {
+    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(v, &(*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)
@@ -106,15 +105,14 @@  visit_type_%(c_type)s(v, &(*obj)->%(c_name)s, "%(name)s", &err);
         if memb.optional:
             pop_indent()
             ret += mcgen('''
-}
+    }
 ''')
         ret += mcgen('''
-if (err) {
-    goto out;
-}
+    if (err) {
+        goto out;
+    }
 ''')

-    pop_indent()
     if re.search('^ *goto out;', ret, re.MULTILINE):
         ret += mcgen('''