[v2,4/6] qapi: Disentangle QAPIDoc code
diff mbox series

Message ID 20190517144232.18965-5-kwolf@redhat.com
State New
Headers show
Series
  • file-posix: Add dynamic-auto-read-only QAPI feature
Related show

Commit Message

Kevin Wolf May 17, 2019, 2:42 p.m. UTC
Documentation comment follow a certain structure: First, we have a text
with a general description (called QAPIDoc.body). After this,
descriptions of the arguments follow. Finally, we have part that
contains various named sections.

The code doesn't show this structure but just checks the right side
conditions so it happens to do the right set of things in the right
phase. This is hard to follow, and adding support for documentation of
features would be even harder.

This restructures the code so that the three parts are clearly
separated. The code becomes a bit longer, but easier to follow.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 scripts/qapi/common.py | 107 ++++++++++++++++++++++++++++++++---------
 1 file changed, 83 insertions(+), 24 deletions(-)

Comments

Markus Armbruster May 24, 2019, 4:11 p.m. UTC | #1
Kevin Wolf <kwolf@redhat.com> writes:

> Documentation comment follow a certain structure: First, we have a text
> with a general description (called QAPIDoc.body). After this,
> descriptions of the arguments follow. Finally, we have part that
> contains various named sections.
>
> The code doesn't show this structure but just checks the right side
> conditions so it happens to do the right set of things in the right

What are "side conditions"?

> phase. This is hard to follow, and adding support for documentation of
> features would be even harder.
>
> This restructures the code so that the three parts are clearly
> separated. The code becomes a bit longer, but easier to follow.

Recommend to mention that output remains unchanged.

>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  scripts/qapi/common.py | 107 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 83 insertions(+), 24 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 71944e2e30..1d0f4847db 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -120,6 +120,27 @@ class QAPIDoc(object):
>          def connect(self, member):
>              self.member = member
>  
> +    class SymbolPart:
> +        """
> +        Describes which part of the documentation we're parsing right now.

So SymbolPart is a part of the documentation.  Shouldn't it be named
DocPart then?

> +
> +        BODY means that we're ready to process freeform text into self.body. A

s/freeform/free-form/

> +        symbol name is only allowed if no other text was parsed yet. It is

Start your sentences with a capital letter.

> +        interpreted as the symbol name that describes the currently documented
> +        object. On getting the second symbol name, we proceed to ARGS.
> +
> +        ARGS means that we're parsing the arguments section. Any symbol name is
> +        interpreted as an argument and an ArgSection is created for it.
> +
> +        VARIOUS is the final part where freeform sections may appear. This
> +        includes named sections such as "Return:" as well as unnamed
> +        paragraphs. No symbols are allowed any more in this part.

s/any more/anymore/

Suggest "Symbols are not allowed anymore".

Only expression documentation blocks have these parts.  Free-form
documentation blocks don't.  Peeking ahead at the parent class's code:
we make the complete free-form doc block a BODY part, which is okay.

> +        """

Considering the dearth of doc strings in this code, criticizing yours
makes me feel bad, but here goes anyway.  This class is not about
parsing.  Pretty much all of the doc string really belongs to the parent
class, or maybe to its .append().  It may well also belong to
qapi-code-gen.txt section "Expression documentation".

Speaking of that section: it could use some love, to put it charitably.
Mind, I'm not asking you to give it your love ;)

Let me try to write a doc string that actually belongs here:

        """
        A documentation part.

        Expression documentation blocks consist of
        * a BODY part: first line naming the expression, plus an
          optional overview
        * an ARGS part: description of each argument (for commands and
          events) or member (for structs, unions and alternates),
        * a VARIOUS part: optional tagged sections.

        Free-form documentation blocks consist only of a BODY part.
        """

Could throw in a pointer to qapi-code-gen.txt.

This loses (useful!) documentation on how we interpret "symbol names".
Should move to the code that actually deals with them.

> +        # Can't make it a subclass of Enum because of Python 2

Thanks for documenting Python 2 contortions!  Let's phrase it as a TODO
comment.

> +        BODY = 0

Any particular reason for 0?

As far as I can tell, Python enum values commonly start with 1, to make
them all true.

> +        ARGS = 1
> +        VARIOUS = 2
> +
>      def __init__(self, parser, info):
>          # self._parser is used to report errors with QAPIParseError.  The
>          # resulting error position depends on the state of the parser.
> @@ -135,6 +156,7 @@ class QAPIDoc(object):
>          self.sections = []
>          # the current section
>          self._section = self.body
> +        self._part = QAPIDoc.SymbolPart.BODY

The right hand side is tiresome, but I don't have better ideas.

>  
>      def has_section(self, name):
>          """Return True if we have a section with this name."""
> @@ -154,37 +176,84 @@ class QAPIDoc(object):
       def append(self, line):
           """Parse a comment line and add it to the documentation."""
           line = line[1:]
           if not line:
               self._append_freeform(line)
               return

           if line[0] != ' ':
>              raise QAPIParseError(self._parser, "Missing space after #")
>          line = line[1:]
>  
> +        if self._part == QAPIDoc.SymbolPart.BODY:
> +            self._append_body_line(line)
> +        elif self._part == QAPIDoc.SymbolPart.ARGS:
> +            self._append_args_line(line)
> +        elif self._part == QAPIDoc.SymbolPart.VARIOUS:
> +            self._append_various_line(line)
> +        else:
> +            assert False

Hmm.  As far as I can tell, this what we use ._part for.  All other
occurences assign to it.

If you replace

    self._part = QAPIDoc.SymbolPart.BODY

by

    self._append_line = self._append_body_line

and so forth, then the whole conditional shrinks to

    self._append_line(line)

and we don't have to muck around with enums.

> +
> +

pycodestyle-3 gripes:

    scripts/qapi/common.py:196:5: E303 too many blank lines (2)

> +    def end_comment(self):
> +        self._end_section()
> +
> +    def _check_named_section(self, line, name):
> +        if name in ('Returns:', 'Since:',
> +                    # those are often singular or plural
> +                    'Note:', 'Notes:',
> +                    'Example:', 'Examples:',
> +                    'TODO:'):
> +            self._part = QAPIDoc.SymbolPart.VARIOUS

Hiding such a side effect in a _check_FOO() function isn't so nice, but
considering the code you're cleaning up, you got a fairly generous "not
so nice" allowance to spend.  No need to do anything.

> +            return True
> +        return False

@line is unused.

> +
> +    def _append_body_line(self, line):
> +        name = line.split(' ', 1)[0]
>          # FIXME not nice: things like '#  @foo:' and '# @foo: ' aren't
>          # recognized, and get silently treated as ordinary text
> -        if self.symbol:
> -            self._append_symbol_line(line)
> -        elif not self.body.text and line.startswith('@'):
> +        if not self.symbol and not self.body.text and line.startswith('@'):
>              if not line.endswith(':'):
>                  raise QAPIParseError(self._parser, "Line should end with :")
>              self.symbol = line[1:-1]
>              # FIXME invalid names other than the empty string aren't flagged
>              if not self.symbol:
>                  raise QAPIParseError(self._parser, "Invalid name")
> +        elif self.symbol:
> +            # We already know that we document some symbol
> +            if name.startswith('@') and name.endswith(':'):
> +                self._part = QAPIDoc.SymbolPart.ARGS
> +                self._append_args_line(line)
> +            elif self.symbol and self._check_named_section(line, name):
> +                self._append_various_line(line)
> +            else:
> +                self._append_freeform(line.strip())
>          else:
> -            self._append_freeform(line)
> -
> -    def end_comment(self):
> -        self._end_section()
> +            # This is free-form documentation without a symbol
> +            self._append_freeform(line.strip())
>  
> -    def _append_symbol_line(self, line):
> +    def _append_args_line(self, line):
>          name = line.split(' ', 1)[0]
>  
>          if name.startswith('@') and name.endswith(':'):
>              line = line[len(name)+1:]
>              self._start_args_section(name[1:-1])
> -        elif name in ('Returns:', 'Since:',
> -                      # those are often singular or plural
> -                      'Note:', 'Notes:',
> -                      'Example:', 'Examples:',
> -                      'TODO:'):
> +        elif self._check_named_section(line, name):
> +            return self._append_various_line(line)

Here you return something, and...

> +        elif (self._section.text.endswith('\n\n')
> +              and line and not line[0].isspace()):
> +            self._start_section()
> +            self._part = QAPIDoc.SymbolPart.VARIOUS
> +            return self._append_various_line(line)

... also here, but ...

> +
> +        self._append_freeform(line.strip())

... not here.

> +
> +    def _append_various_line(self, line):
> +        name = line.split(' ', 1)[0]
> +
> +        if name.startswith('@') and name.endswith(':'):
> +            raise QAPIParseError(self._parser,
> +                                 "'%s' can't follow '%s' section"
> +                                 % (name, self.sections[0].name))
> +        elif self._check_named_section(line, name):
>              line = line[len(name)+1:]
>              self._start_section(name[:-1])
>  
> +        if (not self._section.name or
> +            not self._section.name.startswith('Example')):

pycodestyle-3 gripes:

    scripts/qapi/common.py:283:13: E129 visually indented line with same indent as next logical line

Fix like this:

           if (not self._section.name
                   or not self._section.name.startswith('Example')):

> +            line = line.strip()
> +
>          self._append_freeform(line)
>  
>      def _start_args_section(self, name):
> @@ -194,10 +263,7 @@ class QAPIDoc(object):
>          if name in self.args:
>              raise QAPIParseError(self._parser,
>                                   "'%s' parameter name duplicated" % name)
> -        if self.sections:
> -            raise QAPIParseError(self._parser,
> -                                 "'@%s:' can't follow '%s' section"
> -                                 % (name, self.sections[0].name))
> +        assert not self.sections
>          self._end_section()
>          self._section = QAPIDoc.ArgSection(name)
>          self.args[name] = self._section
> @@ -219,13 +285,6 @@ class QAPIDoc(object):
>              self._section = None
>  
>      def _append_freeform(self, line):
> -        in_arg = isinstance(self._section, QAPIDoc.ArgSection)
> -        if (in_arg and self._section.text.endswith('\n\n')
> -                and line and not line[0].isspace()):
> -            self._start_section()
> -        if (in_arg or not self._section.name
> -                or not self._section.name.startswith('Example')):
> -            line = line.strip()
>          match = re.match(r'(@\S+:)', line)
>          if match:
>              raise QAPIParseError(self._parser,

The patch is hard to read (not your fault).  I mostly reviewed the
result instead.
Kevin Wolf May 29, 2019, 10:09 p.m. UTC | #2
Am 24.05.2019 um 18:11 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Documentation comment follow a certain structure: First, we have a text
> > with a general description (called QAPIDoc.body). After this,
> > descriptions of the arguments follow. Finally, we have part that
> > contains various named sections.
> >
> > The code doesn't show this structure but just checks the right side
> > conditions so it happens to do the right set of things in the right
> 
> What are "side conditions"?
> 
> > phase. This is hard to follow, and adding support for documentation of
> > features would be even harder.
> >
> > This restructures the code so that the three parts are clearly
> > separated. The code becomes a bit longer, but easier to follow.
> 
> Recommend to mention that output remains unchanged.
> 
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  scripts/qapi/common.py | 107 ++++++++++++++++++++++++++++++++---------
> >  1 file changed, 83 insertions(+), 24 deletions(-)
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index 71944e2e30..1d0f4847db 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -120,6 +120,27 @@ class QAPIDoc(object):
> >          def connect(self, member):
> >              self.member = member
> >  
> > +    class SymbolPart:
> > +        """
> > +        Describes which part of the documentation we're parsing right now.
> 
> So SymbolPart is a part of the documentation.  Shouldn't it be named
> DocPart then?

That's a better name. I was stuck in the old code (which was concerned
about what a symbol name means at which point) rather than thinking
about high-level concepts.

> > +
> > +        BODY means that we're ready to process freeform text into self.body. A
> 
> s/freeform/free-form/

Both are valid spellings and I generally don't expect correct spellings
to be corrected, but arguably "free-form" is more standard. I'll change
it. (If we were consistent, the method should have been named
_append_free_form rather than _append_freeform originally...)

> > +        symbol name is only allowed if no other text was parsed yet. It is
> 
> Start your sentences with a capital letter.

I would gladly correct a sentence not starting with a capital letter if
I could see any. The quoted sentence starts with a capital "A" in the
previous line.

> > +        interpreted as the symbol name that describes the currently documented
> > +        object. On getting the second symbol name, we proceed to ARGS.
> > +
> > +        ARGS means that we're parsing the arguments section. Any symbol name is
> > +        interpreted as an argument and an ArgSection is created for it.
> > +
> > +        VARIOUS is the final part where freeform sections may appear. This
> > +        includes named sections such as "Return:" as well as unnamed
> > +        paragraphs. No symbols are allowed any more in this part.
> 
> s/any more/anymore/

Again both are valid, but this time, "any more" is the more standard
spelling.

> > +        # Can't make it a subclass of Enum because of Python 2
> 
> Thanks for documenting Python 2 contortions!  Let's phrase it as a TODO
> comment.
> 
> > +        BODY = 0
> 
> Any particular reason for 0?
> 
> As far as I can tell, Python enum values commonly start with 1, to make
> them all true.

If you use enums in a boolean context, you're doing something wrong
anyway. *shrug*

I'll change it, it's consistent with real Enum classes where the values
becomes non-integer objects (which therefore evaluate as True in boolean
contexts).

> > +        ARGS = 1
> > +        VARIOUS = 2
> > +
> >      def __init__(self, parser, info):
> >          # self._parser is used to report errors with QAPIParseError.  The
> >          # resulting error position depends on the state of the parser.
> > @@ -135,6 +156,7 @@ class QAPIDoc(object):
> >          self.sections = []
> >          # the current section
> >          self._section = self.body
> > +        self._part = QAPIDoc.SymbolPart.BODY
> 
> The right hand side is tiresome, but I don't have better ideas.

This is just what Python enums look like... I could move the class
outside of QAPIDoc to save that part of the prefix, but I'd prefer not
to.

> >  
> >      def has_section(self, name):
> >          """Return True if we have a section with this name."""
> > @@ -154,37 +176,84 @@ class QAPIDoc(object):
>        def append(self, line):
>            """Parse a comment line and add it to the documentation."""
>            line = line[1:]
>            if not line:
>                self._append_freeform(line)
>                return
> 
>            if line[0] != ' ':
> >              raise QAPIParseError(self._parser, "Missing space after #")
> >          line = line[1:]
> >  
> > +        if self._part == QAPIDoc.SymbolPart.BODY:
> > +            self._append_body_line(line)
> > +        elif self._part == QAPIDoc.SymbolPart.ARGS:
> > +            self._append_args_line(line)
> > +        elif self._part == QAPIDoc.SymbolPart.VARIOUS:
> > +            self._append_various_line(line)
> > +        else:
> > +            assert False
> 
> Hmm.  As far as I can tell, this what we use ._part for.  All other
> occurences assign to it.
> 
> If you replace
> 
>     self._part = QAPIDoc.SymbolPart.BODY
> 
> by
> 
>     self._append_line = self._append_body_line
> 
> and so forth, then the whole conditional shrinks to
> 
>     self._append_line(line)
> 
> and we don't have to muck around with enums.

I could just have added a boolean that decides whether a symbol is an
argument or a feature. That would have been a minimal hack that
wouldn't involve any enums.

I intentionally decided not to do that because the whole structure of
the parser was horribly confusing to me and I felt that introducing a
clear state machine would improve its legibility a lot. I still think
that this is what it did.

If you don't like a proper state machine, I can do that bool thing. I
don't think throwing in function pointers would be very helpful for
readers, so we'd get a major code change for no gain.

Kevin
Markus Armbruster June 3, 2019, 8:09 a.m. UTC | #3
Cc: Eric for English language advice.

Kevin Wolf <kwolf@redhat.com> writes:

> Am 24.05.2019 um 18:11 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Documentation comment follow a certain structure: First, we have a text
>> > with a general description (called QAPIDoc.body). After this,
>> > descriptions of the arguments follow. Finally, we have part that
>> > contains various named sections.
>> >
>> > The code doesn't show this structure but just checks the right side
>> > conditions so it happens to do the right set of things in the right
>> 
>> What are "side conditions"?
>> 
>> > phase. This is hard to follow, and adding support for documentation of
>> > features would be even harder.
>> >
>> > This restructures the code so that the three parts are clearly
>> > separated. The code becomes a bit longer, but easier to follow.
>> 
>> Recommend to mention that output remains unchanged.
>> 
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> >  scripts/qapi/common.py | 107 ++++++++++++++++++++++++++++++++---------
>> >  1 file changed, 83 insertions(+), 24 deletions(-)
>> >
>> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> > index 71944e2e30..1d0f4847db 100644
>> > --- a/scripts/qapi/common.py
>> > +++ b/scripts/qapi/common.py
>> > @@ -120,6 +120,27 @@ class QAPIDoc(object):
>> >          def connect(self, member):
>> >              self.member = member
>> >  
>> > +    class SymbolPart:
>> > +        """
>> > +        Describes which part of the documentation we're parsing right now.
>> 
>> So SymbolPart is a part of the documentation.  Shouldn't it be named
>> DocPart then?
>
> That's a better name. I was stuck in the old code (which was concerned
> about what a symbol name means at which point) rather than thinking
> about high-level concepts.
>
>> > +
>> > +        BODY means that we're ready to process freeform text into self.body. A
>> 
>> s/freeform/free-form/
>
> Both are valid spellings and I generally don't expect correct spellings
> to be corrected, but arguably "free-form" is more standard. I'll change
> it.

The error message and qapi-code-gen.txt consistently spell it free-form.
I prefer consistent spelling, not least for greppability.

>     (If we were consistent, the method should have been named
> _append_free_form rather than _append_freeform originally...)

Yes.

>> > +        symbol name is only allowed if no other text was parsed yet. It is
>> 
>> Start your sentences with a capital letter.
>
> I would gladly correct a sentence not starting with a capital letter if
> I could see any. The quoted sentence starts with a capital "A" in the
> previous line.

My mistake, I overlooked the "A" at the end of the line.

>> > +        interpreted as the symbol name that describes the currently documented
>> > +        object. On getting the second symbol name, we proceed to ARGS.
>> > +
>> > +        ARGS means that we're parsing the arguments section. Any symbol name is
>> > +        interpreted as an argument and an ArgSection is created for it.
>> > +
>> > +        VARIOUS is the final part where freeform sections may appear. This
>> > +        includes named sections such as "Return:" as well as unnamed
>> > +        paragraphs. No symbols are allowed any more in this part.
>> 
>> s/any more/anymore/
>
> Again both are valid, but this time, "any more" is the more standard
> spelling.

Eric, what's your take?

>> > +        # Can't make it a subclass of Enum because of Python 2
>> 
>> Thanks for documenting Python 2 contortions!  Let's phrase it as a TODO
>> comment.
>> 
>> > +        BODY = 0
>> 
>> Any particular reason for 0?
>> 
>> As far as I can tell, Python enum values commonly start with 1, to make
>> them all true.
>
> If you use enums in a boolean context, you're doing something wrong
> anyway. *shrug*

No argument.  But...

> I'll change it, it's consistent with real Enum classes where the values
> becomes non-integer objects (which therefore evaluate as True in boolean
> contexts).

... consistency with real Enum costs us nothing, so let's do it.

>> > +        ARGS = 1
>> > +        VARIOUS = 2
>> > +
>> >      def __init__(self, parser, info):
>> >          # self._parser is used to report errors with QAPIParseError.  The
>> >          # resulting error position depends on the state of the parser.
>> > @@ -135,6 +156,7 @@ class QAPIDoc(object):
>> >          self.sections = []
>> >          # the current section
>> >          self._section = self.body
>> > +        self._part = QAPIDoc.SymbolPart.BODY
>> 
>> The right hand side is tiresome, but I don't have better ideas.
>
> This is just what Python enums look like... I could move the class
> outside of QAPIDoc to save that part of the prefix, but I'd prefer not
> to.

It's okay.

>> >  
>> >      def has_section(self, name):
>> >          """Return True if we have a section with this name."""
>> > @@ -154,37 +176,84 @@ class QAPIDoc(object):
>>        def append(self, line):
>>            """Parse a comment line and add it to the documentation."""
>>            line = line[1:]
>>            if not line:
>>                self._append_freeform(line)
>>                return
>> 
>>            if line[0] != ' ':
>> >              raise QAPIParseError(self._parser, "Missing space after #")
>> >          line = line[1:]
>> >  
>> > +        if self._part == QAPIDoc.SymbolPart.BODY:
>> > +            self._append_body_line(line)
>> > +        elif self._part == QAPIDoc.SymbolPart.ARGS:
>> > +            self._append_args_line(line)
>> > +        elif self._part == QAPIDoc.SymbolPart.VARIOUS:
>> > +            self._append_various_line(line)
>> > +        else:
>> > +            assert False
>> 
>> Hmm.  As far as I can tell, this what we use ._part for.  All other
>> occurences assign to it.
>> 
>> If you replace
>> 
>>     self._part = QAPIDoc.SymbolPart.BODY
>> 
>> by
>> 
>>     self._append_line = self._append_body_line
>> 
>> and so forth, then the whole conditional shrinks to
>> 
>>     self._append_line(line)
>> 
>> and we don't have to muck around with enums.
>
> I could just have added a boolean that decides whether a symbol is an
> argument or a feature. That would have been a minimal hack that
> wouldn't involve any enums.
>
> I intentionally decided not to do that because the whole structure of
> the parser was horribly confusing to me

Not just to you.

>                                         and I felt that introducing a
> clear state machine would improve its legibility a lot. I still think
> that this is what it did.
>
> If you don't like a proper state machine, I can do that bool thing. I
> don't think throwing in function pointers would be very helpful for
> readers, so we'd get a major code change for no gain.

There's more than one way to code a state machine.  Encoding the state
as enum / switch over next state is one.  Encoding the state as pointer
/ jump to next state is another.

Patch
diff mbox series

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 71944e2e30..1d0f4847db 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -120,6 +120,27 @@  class QAPIDoc(object):
         def connect(self, member):
             self.member = member
 
+    class SymbolPart:
+        """
+        Describes which part of the documentation we're parsing right now.
+
+        BODY means that we're ready to process freeform text into self.body. A
+        symbol name is only allowed if no other text was parsed yet. It is
+        interpreted as the symbol name that describes the currently documented
+        object. On getting the second symbol name, we proceed to ARGS.
+
+        ARGS means that we're parsing the arguments section. Any symbol name is
+        interpreted as an argument and an ArgSection is created for it.
+
+        VARIOUS is the final part where freeform sections may appear. This
+        includes named sections such as "Return:" as well as unnamed
+        paragraphs. No symbols are allowed any more in this part.
+        """
+        # Can't make it a subclass of Enum because of Python 2
+        BODY = 0
+        ARGS = 1
+        VARIOUS = 2
+
     def __init__(self, parser, info):
         # self._parser is used to report errors with QAPIParseError.  The
         # resulting error position depends on the state of the parser.
@@ -135,6 +156,7 @@  class QAPIDoc(object):
         self.sections = []
         # the current section
         self._section = self.body
+        self._part = QAPIDoc.SymbolPart.BODY
 
     def has_section(self, name):
         """Return True if we have a section with this name."""
@@ -154,37 +176,84 @@  class QAPIDoc(object):
             raise QAPIParseError(self._parser, "Missing space after #")
         line = line[1:]
 
+        if self._part == QAPIDoc.SymbolPart.BODY:
+            self._append_body_line(line)
+        elif self._part == QAPIDoc.SymbolPart.ARGS:
+            self._append_args_line(line)
+        elif self._part == QAPIDoc.SymbolPart.VARIOUS:
+            self._append_various_line(line)
+        else:
+            assert False
+
+
+    def end_comment(self):
+        self._end_section()
+
+    def _check_named_section(self, line, name):
+        if name in ('Returns:', 'Since:',
+                    # those are often singular or plural
+                    'Note:', 'Notes:',
+                    'Example:', 'Examples:',
+                    'TODO:'):
+            self._part = QAPIDoc.SymbolPart.VARIOUS
+            return True
+        return False
+
+    def _append_body_line(self, line):
+        name = line.split(' ', 1)[0]
         # FIXME not nice: things like '#  @foo:' and '# @foo: ' aren't
         # recognized, and get silently treated as ordinary text
-        if self.symbol:
-            self._append_symbol_line(line)
-        elif not self.body.text and line.startswith('@'):
+        if not self.symbol and not self.body.text and line.startswith('@'):
             if not line.endswith(':'):
                 raise QAPIParseError(self._parser, "Line should end with :")
             self.symbol = line[1:-1]
             # FIXME invalid names other than the empty string aren't flagged
             if not self.symbol:
                 raise QAPIParseError(self._parser, "Invalid name")
+        elif self.symbol:
+            # We already know that we document some symbol
+            if name.startswith('@') and name.endswith(':'):
+                self._part = QAPIDoc.SymbolPart.ARGS
+                self._append_args_line(line)
+            elif self.symbol and self._check_named_section(line, name):
+                self._append_various_line(line)
+            else:
+                self._append_freeform(line.strip())
         else:
-            self._append_freeform(line)
-
-    def end_comment(self):
-        self._end_section()
+            # This is free-form documentation without a symbol
+            self._append_freeform(line.strip())
 
-    def _append_symbol_line(self, line):
+    def _append_args_line(self, line):
         name = line.split(' ', 1)[0]
 
         if name.startswith('@') and name.endswith(':'):
             line = line[len(name)+1:]
             self._start_args_section(name[1:-1])
-        elif name in ('Returns:', 'Since:',
-                      # those are often singular or plural
-                      'Note:', 'Notes:',
-                      'Example:', 'Examples:',
-                      'TODO:'):
+        elif self._check_named_section(line, name):
+            return self._append_various_line(line)
+        elif (self._section.text.endswith('\n\n')
+              and line and not line[0].isspace()):
+            self._start_section()
+            self._part = QAPIDoc.SymbolPart.VARIOUS
+            return self._append_various_line(line)
+
+        self._append_freeform(line.strip())
+
+    def _append_various_line(self, line):
+        name = line.split(' ', 1)[0]
+
+        if name.startswith('@') and name.endswith(':'):
+            raise QAPIParseError(self._parser,
+                                 "'%s' can't follow '%s' section"
+                                 % (name, self.sections[0].name))
+        elif self._check_named_section(line, name):
             line = line[len(name)+1:]
             self._start_section(name[:-1])
 
+        if (not self._section.name or
+            not self._section.name.startswith('Example')):
+            line = line.strip()
+
         self._append_freeform(line)
 
     def _start_args_section(self, name):
@@ -194,10 +263,7 @@  class QAPIDoc(object):
         if name in self.args:
             raise QAPIParseError(self._parser,
                                  "'%s' parameter name duplicated" % name)
-        if self.sections:
-            raise QAPIParseError(self._parser,
-                                 "'@%s:' can't follow '%s' section"
-                                 % (name, self.sections[0].name))
+        assert not self.sections
         self._end_section()
         self._section = QAPIDoc.ArgSection(name)
         self.args[name] = self._section
@@ -219,13 +285,6 @@  class QAPIDoc(object):
             self._section = None
 
     def _append_freeform(self, line):
-        in_arg = isinstance(self._section, QAPIDoc.ArgSection)
-        if (in_arg and self._section.text.endswith('\n\n')
-                and line and not line[0].isspace()):
-            self._start_section()
-        if (in_arg or not self._section.name
-                or not self._section.name.startswith('Example')):
-            line = line.strip()
         match = re.match(r'(@\S+:)', line)
         if match:
             raise QAPIParseError(self._parser,