Re: [Qemu-devel] [PATCH v2 4/6] qapi: Disentangle QAPIDoc code

From: Markus Armbruster <armbru@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: pkrempa@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org
Subject: Re: [Qemu-devel] [PATCH v2 4/6] qapi: Disentangle QAPIDoc code
Date: Mon, 03 Jun 2019 10:09:53 +0200	[thread overview]
Message-ID: <8736krum26.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20190529220915.GA3471@localhost.localdomain> (Kevin Wolf's message of "Thu, 30 May 2019 00:09:15 +0200")

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.