[PATCH 0/2] qapi: A section heading bug fix, and maybe an improvement

[PATCH 0/2] qapi: A section heading bug fix, and maybe an improvement

[Markus Armbruster]

PATCH 1 fixes an old defect in the doc comment parser.  I figure it'll
simplify the rST generator's job.

PATCH 2 might simplify it further.  It's RFC because I'm not sure it
does.  Peter, you tell me :)


Markus Armbruster (2):
  qapi: Reject section markup in definition documentation
  [RFC] qapi: Make section headings start a new doc comment block

 docs/devel/qapi-code-gen.txt           |  2 ++
 scripts/qapi/parser.py                 | 24 +++++++++++++++++-------
 tests/qapi-schema/doc-bad-section.err  |  1 +
 tests/qapi-schema/doc-bad-section.json |  3 +--
 tests/qapi-schema/doc-bad-section.out  | 24 ------------------------
 tests/qapi-schema/doc-good.out         |  3 ++-
 6 files changed, 23 insertions(+), 34 deletions(-)

-- 
2.21.1

[PATCH 1/2] qapi: Reject section markup in definition documentation

[Markus Armbruster]

Section markup in definition documentation makes no sense and can
produce invalid Texinfo.  Reject.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/devel/qapi-code-gen.txt           |  2 ++
 scripts/qapi/parser.py                 |  5 +++++
 tests/qapi-schema/doc-bad-section.err  |  1 +
 tests/qapi-schema/doc-bad-section.json |  3 +--
 tests/qapi-schema/doc-bad-section.out  | 24 ------------------------
 5 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 1967adfa92..382a672874 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -835,6 +835,8 @@ Double the '=' for a subsection title:
 
     # == Subsection title
 
+Both are only permitted in free-form documentation.
+
 '|' denotes examples:
 
     # | Text of the example, may span
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index abadacbb0e..f12c67d7d2 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -282,6 +282,11 @@ class QAPISchemaParser:
                 doc.end_comment()
                 self.accept()
                 return doc
+            if self.val.startswith('# ='):
+                if doc.symbol:
+                    raise QAPIParseError(
+                        self,
+                        "unexpected '=' markup in definition documentation")
             doc.append(self.val)
             self.accept(False)
 
diff --git a/tests/qapi-schema/doc-bad-section.err b/tests/qapi-schema/doc-bad-section.err
index e69de29bb2..785cacc08c 100644
--- a/tests/qapi-schema/doc-bad-section.err
+++ b/tests/qapi-schema/doc-bad-section.err
@@ -0,0 +1 @@
+doc-bad-section.json:5:1: unexpected '=' markup in definition documentation
diff --git a/tests/qapi-schema/doc-bad-section.json b/tests/qapi-schema/doc-bad-section.json
index 560df4b087..8175d95867 100644
--- a/tests/qapi-schema/doc-bad-section.json
+++ b/tests/qapi-schema/doc-bad-section.json
@@ -1,9 +1,8 @@
 # = section within an expression comment
-# BUG: not rejected
 
 ##
 # @Enum:
-# == Produces *invalid* texinfo
+# == No good here
 # @one: The _one_ {and only}
 #
 # @two is undocumented
diff --git a/tests/qapi-schema/doc-bad-section.out b/tests/qapi-schema/doc-bad-section.out
index 367e2a1c3e..e69de29bb2 100644
--- a/tests/qapi-schema/doc-bad-section.out
+++ b/tests/qapi-schema/doc-bad-section.out
@@ -1,24 +0,0 @@
-module None
-object q_empty
-enum QType
-    prefix QTYPE
-    member none
-    member qnull
-    member qnum
-    member qstring
-    member qdict
-    member qlist
-    member qbool
-module doc-bad-section.json
-enum Enum
-    member one
-    member two
-doc symbol=Enum
-    body=
-== Produces *invalid* texinfo
-    arg=one
-The _one_ {and only}
-    arg=two
-
-    section=None
-@two is undocumented
-- 
2.21.1

[PATCH RFC 2/2] qapi: Make section headings start a new doc comment block

[Markus Armbruster]

Our current QAPI doc-comment markup allows section headers (introduced
with a leading '=' or '==') anywhere in a free-form documentation
comment.  This works for Texinfo because the generator simply prints a
Texinfo section command at that point in the output stream.  For rST
generation, since we're assembling a tree of docutils nodes, this is
awkward because a new section implies starting a new section node at
the top level of the tree and generating text into there.

Make section headers start a new free-form documentation block, so the
future rST document generator doesn't have to look at every line in
free-form blocks and handle headings in odd places.

This change makes no difference to the generated Texinfo.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/parser.py         | 21 +++++++++++++--------
 tests/qapi-schema/doc-good.out |  3 ++-
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index f12c67d7d2..165925ca72 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -52,8 +52,8 @@ class QAPISchemaParser:
             info = self.info
             if self.tok == '#':
                 self.reject_expr_doc(cur_doc)
-                cur_doc = self.get_doc(info)
-                self.docs.append(cur_doc)
+                for cur_doc in self.get_doc(info):
+                    self.docs.append(cur_doc)
                 continue
 
             expr = self.get_expr(False)
@@ -270,7 +270,8 @@ class QAPISchemaParser:
             raise QAPIParseError(
                 self, "junk after '##' at start of documentation comment")
 
-        doc = QAPIDoc(self, info)
+        docs = []
+        cur_doc = QAPIDoc(self, info)
         self.accept(False)
         while self.tok == '#':
             if self.val.startswith('##'):
@@ -279,15 +280,20 @@ class QAPISchemaParser:
                     raise QAPIParseError(
                         self,
                         "junk after '##' at end of documentation comment")
-                doc.end_comment()
+                cur_doc.end_comment()
+                docs.append(cur_doc)
                 self.accept()
-                return doc
+                return docs
             if self.val.startswith('# ='):
-                if doc.symbol:
+                if cur_doc.symbol:
                     raise QAPIParseError(
                         self,
                         "unexpected '=' markup in definition documentation")
-            doc.append(self.val)
+                if cur_doc.body.text:
+                    cur_doc.end_comment()
+                    docs.append(cur_doc)
+                    cur_doc = QAPIDoc(self, info)
+            cur_doc.append(self.val)
             self.accept(False)
 
         raise QAPIParseError(self, "documentation comment must end with '##'")
@@ -316,7 +322,6 @@ class QAPIDoc:
         def __init__(self, name=None):
             # optional section name (argument/member or section name)
             self.name = name
-            # the list of lines for this section
             self.text = ''
 
         def append(self, line):
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index 6757dd26a2..d78a424cd9 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -69,7 +69,8 @@ event EVT-BOXED Object
 doc freeform
     body=
 = Section
-
+doc freeform
+    body=
 == Subsection
 
 *strong* _with emphasis_
-- 
2.21.1

Re: [PATCH 1/2] qapi: Reject section markup in definition documentation

[Eric Blake]

On 3/20/20 4:18 AM, Markus Armbruster wrote:
> Section markup in definition documentation makes no sense and can
> produce invalid Texinfo.  Reject.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   docs/devel/qapi-code-gen.txt           |  2 ++
>   scripts/qapi/parser.py                 |  5 +++++
>   tests/qapi-schema/doc-bad-section.err  |  1 +
>   tests/qapi-schema/doc-bad-section.json |  3 +--
>   tests/qapi-schema/doc-bad-section.out  | 24 ------------------------
>   5 files changed, 9 insertions(+), 26 deletions(-)
> 

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

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

Re: [PATCH RFC 2/2] qapi: Make section headings start a new doc comment block

[Peter Maydell]

On Fri, 20 Mar 2020 at 09:18, Markus Armbruster <armbru@redhat.com> wrote:
>
> Our current QAPI doc-comment markup allows section headers (introduced
> with a leading '=' or '==') anywhere in a free-form documentation
> comment.  This works for Texinfo because the generator simply prints a
> Texinfo section command at that point in the output stream.  For rST
> generation, since we're assembling a tree of docutils nodes, this is
> awkward because a new section implies starting a new section node at
> the top level of the tree and generating text into there.
>
> Make section headers start a new free-form documentation block, so the
> future rST document generator doesn't have to look at every line in
> free-form blocks and handle headings in odd places.
>
> This change makes no difference to the generated Texinfo.

I think this does make things easier for rST generation
(which now can say "if the first line in the freeform doc
is a section heading, do section heading stuff, discard that
line, process rest of freeform doc as normal"), so on
that basis I like it.

I do kind of think it would be overall nicer to go further and
say "section headings are special and not part of free-form doc
comments at all" (both for the doc-comment author by mandating
that they be standalone, and for the consumer of parsed info
by separating section headings out from free-form doc comment
rather than requiring the consumer to say "is this line heading
syntax?"), but that would be more change, so pragmatically
I'm happy if we just do what this patch suggests.

thanks
-- PMM

Re: [PATCH RFC 2/2] qapi: Make section headings start a new doc comment block

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 20 Mar 2020 at 09:18, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Our current QAPI doc-comment markup allows section headers (introduced
>> with a leading '=' or '==') anywhere in a free-form documentation
>> comment.  This works for Texinfo because the generator simply prints a
>> Texinfo section command at that point in the output stream.  For rST
>> generation, since we're assembling a tree of docutils nodes, this is
>> awkward because a new section implies starting a new section node at
>> the top level of the tree and generating text into there.
>>
>> Make section headers start a new free-form documentation block, so the
>> future rST document generator doesn't have to look at every line in
>> free-form blocks and handle headings in odd places.
>>
>> This change makes no difference to the generated Texinfo.
>
> I think this does make things easier for rST generation
> (which now can say "if the first line in the freeform doc
> is a section heading, do section heading stuff, discard that
> line, process rest of freeform doc as normal"), so on
> that basis I like it.

Good.

> I do kind of think it would be overall nicer to go further and
> say "section headings are special and not part of free-form doc
> comments at all" (both for the doc-comment author by mandating
> that they be standalone, and for the consumer of parsed info
> by separating section headings out from free-form doc comment
> rather than requiring the consumer to say "is this line heading
> syntax?"), but that would be more change, so pragmatically
> I'm happy if we just do what this patch suggests.

I think there are two separate issues: doc comment syntax and internal
representation.

Our internal representation reflects the input's flat structure: one
comment block after the other.  I wish it reflected the document's tree
structure instead, but I can't justify the effort to rework it.

What to put into syntax and what to leave to style is often debatable.
Putting headings into their own block makes them stand out even more,
which may be useful.  Baking that into the syntax feels a bit oppressive
to me.  Sometimes a bit of oppression can buy enough consistency to be
worth it.

Thanks!

Re: [PATCH 0/2] qapi: A section heading bug fix, and maybe an improvement

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> PATCH 1 fixes an old defect in the doc comment parser.  I figure it'll
> simplify the rST generator's job.
>
> PATCH 2 might simplify it further.  It's RFC because I'm not sure it
> does.  Peter, you tell me :)

I dropped the ball on this one.  I think both patches make sense, but I
don't want to upset Peter's "Convert QAPI doc comments to generate rST
instead of texinfo" apple cart.  Peter, please tell me what you'd like
me to do:

* Post a pull request, so you can base your v6 on it.

* Nothing, so you can cherry-pick zero, one or both of my patches into
  your v6.

Re: [PATCH 0/2] qapi: A section heading bug fix, and maybe an improvement

[Peter Maydell]

On Mon, 7 Sep 2020 at 15:41, Markus Armbruster <armbru@redhat.com> wrote:
>
> Markus Armbruster <armbru@redhat.com> writes:
>
> > PATCH 1 fixes an old defect in the doc comment parser.  I figure it'll
> > simplify the rST generator's job.
> >
> > PATCH 2 might simplify it further.  It's RFC because I'm not sure it
> > does.  Peter, you tell me :)
>
> I dropped the ball on this one.  I think both patches make sense, but I
> don't want to upset Peter's "Convert QAPI doc comments to generate rST
> instead of texinfo" apple cart.  Peter, please tell me what you'd like
> me to do:
>
> * Post a pull request, so you can base your v6 on it.

Looking back at my comments on the patches I think I was happy
with both and it probably isn't a huge rebase issue, so you
might as well send these in a pullreq. You might consider also
including the first 3 patches from my v5 series, which are just
fixing markup issues in the .json that get detected by the
rst conversion.

thanks
-- PMM

Re: [PATCH 0/2] qapi: A section heading bug fix, and maybe an improvement

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 7 Sep 2020 at 15:41, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>> > PATCH 1 fixes an old defect in the doc comment parser.  I figure it'll
>> > simplify the rST generator's job.
>> >
>> > PATCH 2 might simplify it further.  It's RFC because I'm not sure it
>> > does.  Peter, you tell me :)
>>
>> I dropped the ball on this one.  I think both patches make sense, but I
>> don't want to upset Peter's "Convert QAPI doc comments to generate rST
>> instead of texinfo" apple cart.  Peter, please tell me what you'd like
>> me to do:
>>
>> * Post a pull request, so you can base your v6 on it.
>
> Looking back at my comments on the patches I think I was happy
> with both and it probably isn't a huge rebase issue, so you
> might as well send these in a pullreq. You might consider also
> including the first 3 patches from my v5 series, which are just
> fixing markup issues in the .json that get detected by the
> rst conversion.

Good idea, will do.