Re: [Qemu-devel] [PATCH v7 15/18] qapi: Share gen_err_check()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]

On 10/01/2015 06:40 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> qapi-commands has a nice helper gen_err_check(), but did not
> 
> but does not use
> 
>> use it everywhere. In fact, using it in more places makes it
>> easier to reduce the lines of code used for generating error
>> checks.  This in turn will make it easier for later patches
>> to consolidate another common pattern among the generators.
> 
> Does this message need updating now you're using gen_err_check() less
> aggressively?

No - even though it was used less aggressively, it still made merging
gen_visit_fields() easier, and "using it in more places" does not imply
"in all possible places". So I think, other than the tense correction,
we are okay.

> 
>> The generated code has one less blank line in qapi-event.c
>> functions, but has no semantic difference.
> 
> Is it a stylistic improvement or the opposite?
> 

Hmm. In qapi-visit.c, there are no blank lines between
visit_start_struct() and the matching visit_end_struct(). In
qapi-event.c, pre-patch we had (roughly):

visit_start_struct()
check err

visit fields
...

visit_end_struct()
check err

This patch deleted the first blank line (after visit_start_struct()) but
not the second (prior to visit_end_struct()), so the post-patch looks a
bit lopsided.  But removing both blank lines doesn't seem too bad to me,
if you wanted to squash this in (not sure the line numbers are right):

diff --git i/scripts/qapi-event.py w/scripts/qapi-event.py
index b5e4d59..720486f 100644
--- i/scripts/qapi-event.py
+++ w/scripts/qapi-event.py
@@ -73,7 +73,6 @@ def gen_event_send(name, arg_type):
         ret += gen_err_check()
         ret += gen_visit_fields(arg_type.members, need_cast=True)
         ret += mcgen('''
-
     visit_end_struct(v, &err);
     if (err) {
         goto out;


Or the alternative is to keep generated output unchanged, by keeping the
newline:

diff --git i/scripts/qapi-event.py w/scripts/qapi-event.py
index b5e4d59..d261a46 100644
--- i/scripts/qapi-event.py
+++ w/scripts/qapi-event.py
@@ -71,6 +71,7 @@ def gen_event_send(name, arg_type):
 ''',
                      name=name)
         ret += gen_err_check()
+        ret += '\n'
         ret += gen_visit_fields(arg_type.members, need_cast=True)
         ret += mcgen('''

I'm fine if you want to do either (or nothing) when turning this into a
pull request.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]