[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 4/6] qapi: Disentangle QAPIDoc c
Re: [Qemu-block] [Qemu-devel] [PATCH v2 4/6] qapi: Disentangle QAPIDoc code
Thu, 30 May 2019 00:09:15 +0200
Am 24.05.2019 um 18:11 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> 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 <address@hidden>
> > ---
> > 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
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
> > + 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
> > + # 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
> > + 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
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
> > + 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
> > 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:
> if line != ' ':
> > 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
> self._append_line = self._append_body_line
> and so forth, then the whole conditional shrinks to
> 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.
- [Qemu-block] [PATCH v2 0/6] file-posix: Add dynamic-auto-read-only QAPI feature, Kevin Wolf, 2019/05/17
- [Qemu-block] [PATCH v2 1/6] qapi: Support features for structs, Kevin Wolf, 2019/05/17
- [Qemu-block] [PATCH v2 2/6] tests/qapi-schema: Test for good feature lists in structs, Kevin Wolf, 2019/05/17
- [Qemu-block] [PATCH v2 4/6] qapi: Disentangle QAPIDoc code, Kevin Wolf, 2019/05/17
- [Qemu-block] [PATCH v2 3/6] tests/qapi-schema: Error case tests for features in structs, Kevin Wolf, 2019/05/17
- [Qemu-block] [PATCH v2 5/6] qapi: Allow documentation for features, Kevin Wolf, 2019/05/17
- [Qemu-block] [PATCH v2 6/6] file-posix: Add dynamic-auto-read-only QAPI feature, Kevin Wolf, 2019/05/17
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/6] file-posix: Add dynamic-auto-read-only QAPI feature, Markus Armbruster, 2019/05/24