Re: [PATCH v3 06/13] qapi/parser: remove FIXME comment from _append_body_line

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3173 bytes --]

On Thu, Sep 30, 2021 at 4:47 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > True, we do not check the validity of this symbol -- but we don't check
> > the validity of definition names during parse, either -- that happens
> > later, during the expr check. I don't want to introduce a dependency on
> > expr.py:check_name_str here and introduce a cycle.
> >
> > Instead, rest assured that a documentation block is required for each
> > definition. This requirement uses the names of each section to ensure
> > that we fulfilled this requirement.
> >
> > e.g., let's say that block-core.json has a comment block for
> > "Snapshot!Info" by accident. We'll see this error message:
> >
> > In file included from ../../qapi/block.json:8:
> > ../../qapi/block-core.json: In struct 'SnapshotInfo':
> > ../../qapi/block-core.json:38: documentation comment is for
> 'Snapshot!Info'
> >
> > That's a pretty decent error message.
> >
> > Now, let's say that we actually mangle it twice, identically:
> >
> > ../../qapi/block-core.json: In struct 'Snapshot!Info':
> > ../../qapi/block-core.json:38: struct has an invalid name
> >
> > That's also pretty decent. If we forget to fix it in both places, we'll
> > just be back to the first error.
> >
> > Therefore, let's just drop this FIXME and adjust the error message to
> > not imply a more thorough check than is actually performed.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/parser.py                 | 6 ++++--
> >  tests/qapi-schema/doc-empty-symbol.err | 2 +-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index 2f93a752f66..52748e8e462 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -558,9 +558,11 @@ def _append_body_line(self, line):
> >                  raise QAPIParseError(
> >                      self._parser, "extra whitespace around symbol
> declaration")
> >              self.symbol = line[1:-1]
> > -            # FIXME invalid names other than the empty string aren't
> flagged
> > +            # Invalid names are not checked here, but the name provided
> MUST
> > +            # match the following definition, which *is* validated.
> >              if not self.symbol:
> > -                raise QAPIParseError(self._parser, "invalid name")
> > +                raise QAPIParseError(
> > +                    self._parser, "doc symbol name cannot be blank")
>
> "blank" feels like "empty or just whitespace" to me.  We actually mean
> "empty".
>
> What about "name required after @"?
>
>
Sure, yeah. Updated.


> >          elif self.symbol:
> >              # This is a definition documentation block
> >              name = line.split(' ', 1)[0]
> > diff --git a/tests/qapi-schema/doc-empty-symbol.err
> b/tests/qapi-schema/doc-empty-symbol.err
> > index 81b90e882a7..a4981ee28ea 100644
> > --- a/tests/qapi-schema/doc-empty-symbol.err
> > +++ b/tests/qapi-schema/doc-empty-symbol.err
> > @@ -1 +1 @@
> > -doc-empty-symbol.json:4:1: invalid name
> > +doc-empty-symbol.json:4:1: doc symbol name cannot be blank
>
>

[-- Attachment #2: Type: text/html, Size: 4411 bytes --]