Re: [PATCH v2] qapi/machine: Place the 'Notes' tag after the 'Since' tag

[Philippe Mathieu-Daudé]

On 2/28/20 7:56 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <address@hidden> writes:
> > On 2/27/20 3:55 PM, Philippe Mathieu-Daudé wrote:
> > > On 2/27/20 3:52 PM, Markus Armbruster wrote:
> > > > Philippe Mathieu-Daudé <address@hidden> writes:
> > > >
> > > > > This fixes when adding a 'Since' tag:
> > > > >
> > > > >     In file included from qapi/qapi-schema.json:105:
> > > > >     qapi/machine.json:25:1: '@arch:' can't follow 'Notes' section
> > > >
> > > > I'm confused.  This error is detected in scripts/qapi/parser.py, and it
> > > > is fatal.  Is the build broken for you?  It isn't for me.  Moreover,
> > > > where is @arch?  I can't see it anywhere close to the two spots the
> > > > patch patches.
> > >
> > > I get the error after trying to fix what Eric commented here:
> > > https://www.mail-archive.com/address@hidden/msg682344.html
> >
> > Using:
> > ---
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index 6c11e3cf3a..40a36d6276 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -20,13 +20,15 @@
> >   #        prefix to produce the corresponding QEMU executable name. This
> >   #        is true even for "qemu-system-x86_64".
> >   #
> > +# @rx: since 5.0
> > +#
> >   # Since: 3.0
> >   ##
> >   { 'enum' : 'SysEmuTarget',
> >     'data' : [ 'aarch64', 'alpha', 'arm', 'cris', 'hppa', 'i386', 'lm32',
> >                'm68k', 'microblaze', 'microblazeel', 'mips', 'mips64',
> >                'mips64el', 'mipsel', 'moxie', 'nios2', 'or1k', 'ppc',
> > -             'ppc64', 'riscv32', 'riscv64', 's390x', 'sh4',
> > +             'ppc64', 'riscv32', 'riscv64', 'rx', 's390x', 'sh4',
> >                'sh4eb', 'sparc', 'sparc64', 'tricore', 'unicore32',
> >                'x86_64', 'xtensa', 'xtensaeb' ] }
> > ---
> >
> > or
> >
> > ---
> > diff --git a/qapi/machine.json b/qapi/machine.json
> > index 6c11e3cf3a..4b59e87b6f 100644
> > --- a/qapi/machine.json
> > +++ b/qapi/machine.json
> > @@ -21,12 +21,14 @@
> >   #        is true even for "qemu-system-x86_64".
> >   #
> >   # Since: 3.0
> > +#
> > +# @rx: since 5.0
> >   ##
> >   { 'enum' : 'SysEmuTarget',
> >     'data' : [ 'aarch64', 'alpha', 'arm', 'cris', 'hppa', 'i386', 'lm32',
> >                'm68k', 'microblaze', 'microblazeel', 'mips', 'mips64',
> >                'mips64el', 'mipsel', 'moxie', 'nios2', 'or1k', 'ppc',
> > -             'ppc64', 'riscv32', 'riscv64', 's390x', 'sh4',
> > +             'ppc64', 'riscv32', 'riscv64', 'rx', 's390x', 'sh4',
> >                'sh4eb', 'sparc', 'sparc64', 'tricore', 'unicore32',
> >                'x86_64', 'xtensa', 'xtensaeb' ] }
> > ---
> >
> > I get:
> >
> >    GEN     qapi-gen
> >    GEN     rx-softmmu/config-devices.mak
> > In file included from qapi/qapi-schema.json:105:
> > qapi/machine.json:23:1: '@rx:' can't follow 'Notes' section
> > make: *** [Makefile:645: qapi-gen-timestamp] Error 1
> >
> > This works however:
> >
> > ---
> >   ##
> >   # @SysEmuTarget:
> >   #
> >   # The comprehensive enumeration of QEMU system emulation ("softmmu")
> >   # targets. Run "./configure --help" in the project root directory, and
> >   # look for the *-softmmu targets near the "--target-list" option. The
> >   # individual target constants are not documented here, for the time
> >   # being.
> >   #
> > +# @rx: since 5.0
> > +#
> >   # Notes: The resulting QMP strings can be appended to the "qemu-system-"
> >   #        prefix to produce the corresponding QEMU executable name. This
> >   #        is true even for "qemu-system-x86_64".
> >   #
> >   # Since: 3.0
> >   ##
> >   { 'enum' : 'SysEmuTarget',
> >     'data' : [ 'aarch64', 'alpha', 'arm', 'cris', 'hppa', 'i386', 'lm32',
> >                'm68k', 'microblaze', 'microblazeel', 'mips', 'mips64',
> >                'mips64el', 'mipsel', 'moxie', 'nios2', 'or1k', 'ppc',
> > -             'ppc64', 'riscv32', 'riscv64', 's390x', 'sh4',
> > +             'ppc64', 'riscv32', 'riscv64', 'rx', 's390x', 'sh4',
> >                'sh4eb', 'sparc', 'sparc64', 'tricore', 'unicore32',
> >                'x86_64', 'xtensa', 'xtensaeb' ] }
> > ---
>
> This one adds it to the correct spot.

OK I'll use that then.

> qapi-code-gen.txt:
>
>      Definition documentation starts with a line naming the definition,
>      followed by an optional overview, a description of each argument (for
>      commands and events), member (for structs and unions), branch (for
>      alternates), or value (for enums), and finally optional tagged
>      sections.

I was confused because I understood "@rx: since 5.0" as a tagged 
section, not as an "Optional overview".

> Let's apply this to SysEmuTarget's doc comment:
>
>      ##
>      # @SysEmuTarget:
>
> Line naming the definition
>
>      #
>      # The comprehensive enumeration of QEMU system emulation ("softmmu")
>      # targets. Run "./configure --help" in the project root directory, and
>      # look for the *-softmmu targets near the "--target-list" option. The
>      # individual target constants are not documented here, for the time
>      # being.
>
> Optional overview.
>
> Missing here: a description of each value.  We should enforce such
> descriptions.  We don't, mostly because we have a number of exceptions
> where documentation would be bothersome, such as enum QKeyCode.
>
>      #
>      # Notes: The resulting QMP strings can be appended to the "qemu-system-"
>      #        prefix to produce the corresponding QEMU executable name. This
>      #        is true even for "qemu-system-x86_64".
>
> A tagged section.
>
>      #
>      # Since: 3.0
>
> Another tagged section.
>
>      ##
>
> Note that qapi-code-gen.txt prescribes no order for the tagged
> sections.  They actually occur in pretty much any order:
>
>      $ awk '/^##/ { if (on) { if (length(t) > 1) print t; t="" } on=!on } on && 
> /^# (Returns|Since|Notes?|Examples?|TODO):/ { t=t substr($2, 1, 1) }' qapi/*json | sort | uniq -c
>            1 ENS
>            2 ES
>            1 NES
>            1 NETS
>            1 NNSE
>            1 NRS
>            2 NRSE
>           14 NS
>           11 NSE
>            1 RE
>            6 RES
>            4 RNS
>            6 RNSE
>           23 RS
>           78 RSE
>            1 RSEEEE
>            3 RSEN
>           12 RSNE
>           55 SE
>            4 SN
>            2 SNE
>            2 SRNE
>            1 TRSE
>            1 TS
>            1 TSRE
>
> If you think tightening their order would improve the documentation, we
> should talk.

No thanks :)