qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH RFC 06/21] qapi-gen: New common driver for code


From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH RFC 06/21] qapi-gen: New common driver for code and doc generators
Date: Tue, 6 Feb 2018 08:56:23 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2

On 02/06/2018 01:45 AM, Markus Armbruster wrote:

-def main(argv):
-    (input_file, output_dir, do_c, do_h, prefix, opts) = parse_command_line()
-
-    blurb = '''
- * Schema-defined QAPI/QMP commands
-'''
-
+def gen_commands(schema, output_dir, prefix):
+    blurb = ' * Schema-defined QAPI/QMP commands'

Some churn on the definition of blurb; worth tidying this in the
introduction earlier in the series?

Doesn't seem worth a separate patch, but I can try to reshuffle things
to reduce churn.

Yeah, my question was more about the conversion between multiline
'''...''' to single-line '...'; why not just use the single-line
construct earlier in the series when introducing the blurb variable.

Introduced in PATCH 01:

     -c_comment = '''
     -/*
     - * schema-defined QMP->QAPI command dispatch
     +blurb = '''
     + * Schema-defined QAPI/QMP commands
       *
       * Copyright IBM, Corp. 2011

Multiline made sense here.

Shortened in PATCH 02:

      blurb = '''
       * Schema-defined QAPI/QMP commands
     - *
     - * Copyright IBM, Corp. 2011
     - *
     - * Authors:
     - *  Anthony Liguori   <address@hidden>
       '''


Where it is now a single line...


Variable eliminated there in PATCH 17:

     -        blurb = ' * Schema-defined QAPI types'
     -        self._genc = QAPIGenC(blurb, __doc__)
     -        self._genh = QAPIGenH(blurb, __doc__)
     +        self._module = {}
     +        self._add_module(None, ' * Built-in QAPI types')

Oh, I didn't even notice that!


I could delay the reformatting until PATCH 16, or do it in PATCH 02.
Feels like a wash to me, but if you have a preference...

I guess my preference is to reformat to single line in PATCH 02, then patch 6 doesn't have to touch it, and patch 16 can still get rid of it. But, as it disappears by the end of the series anyways, I'm also okay if you don't bother (churn is not necessarily bad, if it is still easy to review).


The commit message talks only about the QAPI/QMP schema.  It's confusing
because our poor taste in file names creates ambiguity: does
qapi-schema.json refer to ./qapi-schema.json (i.e. the QAPI/QMP one),
qga/qapi-schema.json, or both?

Perhaps I should rename qapi-schema.json to qapi/schema.json.

I'd be in favor of such a change. We'd also rename qga/qapi-schema.json to qga/schema.json?


The commit message needs a note along the lines of "same for
qga/qapi-schema.json".


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



reply via email to

[Prev in Thread] Current Thread [Next in Thread]