On Mon, 17 Feb 2014 09:50:10 +0800
Wenchao Xia <xiaw...@linux.vnet.ibm.com> wrote:
> 于 2014/2/14 17:23, Markus Armbruster 写道:
> > Wenchao Xia <xiaw...@linux.vnet.ibm.com> writes:
> >
> >> 于 2014/2/13 23:14, Markus Armbruster 写道:
> >>> Wenchao Xia <xiaw...@linux.vnet.ibm.com> writes:
> >>>
> >>>> It will check whether the values specified are written correctly,
> >>>> and whether all enum values are covered, when discriminator is a
> >>>> pre-defined enum type
> >>>>
> >>>> Signed-off-by: Wenchao Xia <xiaw...@linux.vnet.ibm.com>
> >>>> Reviewed-by: Eric Blake <ebl...@redhat.com>
> >>>> ---
> >>>> scripts/qapi-visit.py | 17 +++++++++++++++++
> >>>> scripts/qapi.py | 31 +++++++++++++++++++++++++++++++
> >>>> 2 files changed, 48 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> >>>> index 65f1a54..c0efb5f 100644
> >>>> --- a/scripts/qapi-visit.py
> >>>> +++ b/scripts/qapi-visit.py
> >>>> @@ -255,6 +255,23 @@ def generate_visit_union(expr):
> >>>> assert not base
> >>>> return generate_visit_anon_union(name, members)
> >>>>
> >>>> + # If discriminator is specified and it is a pre-defined enum in
> >>>> schema,
> >>>> + # check its correctness
> >>>> + enum_define = discriminator_find_enum_define(expr)
> >>>> + if enum_define:
> >>>> + for key in members:
> >>>> + if not key in enum_define["enum_values"]:
> >>>> + sys.stderr.write("Discriminator value '%s' is not found
> >>>> in "
> >>>> + "enum '%s'\n" %
> >>>> + (key, enum_define["enum_name"]))
> >>>> + sys.exit(1)
> >>>
> >>> Can this happen? If yes, why isn't it diagnosed in qapi.py, like all
> >>> the other semantic errors?
> >>>
> >> I think the parse procedure contains two part:
> >> 1 read qapi-schema.json and parse it into exprs.
> >> 2 translate exprs into final output.
> >> Looking at qapi.py, qapi-visit.py, qapi-types.py, it seems qapi.py is
> >> in charge of step 1 handling literal error, and other two script are in
> >> charge of step 2. The above error can be only detected in step 2 after
> >> all enum defines are remembered in step 1, so I didn't add those things
> >> into qapi.py.
> >
> > The distribution of work between the qapi*py isn't spelled out anywhere,
> > but my working hypothesis is qapi.py is the frontend, and the
> > qapi-{commands,types,visit}.py are backends.
> >
> > The frontend's job is lexical, syntax and semantic analysis.
> >
> > The backends' job is source code generation.
> >
> > This isn't the only possible split, but it's the orthodox way to split
> > compilers.
> >
> >> I guess you want to place the check inside parse_schema() to let
> >> test case detect it easier, one way to go is, let qapi.py do checks
> >> for step 2:
> >>
> >> def parse_schema(fp):
> >> try:
> >> schema = QAPISchema(fp)
> >> except QAPISchemaError, e:
> >> print >>sys.stderr, e
> >> exit(1)
> >>
> >> exprs = []
> >>
> >> for expr in schema.exprs:
> >> if expr.has_key('enum'):
> >> add_enum(expr['enum'])
> >> elif expr.has_key('union'):
> >> add_union(expr)
> >> add_enum('%sKind' % expr['union'])
> >> elif expr.has_key('type'):
> >> add_struct(expr)
> >> exprs.append(expr)
> >>
> >> + for expr in schema.exprs:
> >> + if expr.has_key('union'):
> >> + #check code
> >>
> >> return exprs
> >>
> >> This way qapi.py can detect such errors. Disadvantage is that,
> >> qapi.py is invloved for step 2 things, so some code in qapi.py
> >> and qapi-visit.py may be dupicated, here the "if .... union...
> >> discriminator" code may appear in both qapi.py and qapi-visit.py.
> >
> > How much code would be duplicated?
> >
> Not many now, my concern is it may becomes more complex
> when more check introduced in future.
> However, your distribution of qapi*.py as complier make
> sense, so I am OK to respin this series.
> Luiz, could you apply or push Markus's series, so I
> can pull it as my working base?
Yes, but I have to handle current pull request right now. I'll let you
know when I apply it.
>
>
> >>>> + for key in enum_define["enum_values"]:
> >>>> + if not key in members:
> >>>> + sys.stderr.write("Enum value '%s' is not covered by a branch "
> >>>> + "of union '%s'\n" %
> >>>> + (key, name))
> >>>> + sys.exit(1)
> >>>> +
> >>>
> >>> Likewise.
> >>>
> >>>> ret = generate_visit_enum('%sKind' % name, members.keys())
> >>>>
> >>>> if base:
> >>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
> >>>> index cf34768..0a3ab80 100644
> >>>> --- a/scripts/qapi.py
> >>>> +++ b/scripts/qapi.py
> >>>> @@ -385,3 +385,34 @@ def guardend(name):
> >>>>
> >>>> ''',
> >>>> name=guardname(name))
> >>>> +
> >>
> >> The funtions below are likely helper funtions, I planed to put them
> >> into qapi_helper.py, but they are not much so kepted for easy.
> >
> > That's fine with me.
> >
> >>>> +# This function can be used to check whether "base" is valid
> >>>> +def find_base_fields(base):
> >>>> + base_struct_define = find_struct(base)
> >>>> + if not base_struct_define:
> >>>> + return None
> >>>> + return base_struct_define.get('data')
> >>>> +
> >>>> +# Return the discriminator enum define, if discriminator is specified in
> >>>> +# @expr and it is a pre-defined enum type
> >>>> +def discriminator_find_enum_define(expr):
> >>>> + discriminator = expr.get('discriminator')
> >>>> + base = expr.get('base')
> >>>> +
> >>>> + # Only support discriminator when base present
> >>>> + if not (discriminator and base):
> >>>> + return None
> >>>> +
> >>>> + base_fields = find_base_fields(base)
> >>>> +
> >>>> + if not base_fields:
> >>>> + raise StandardError("Base '%s' is not a valid type\n"
> >>>> + % base)
> >>>
> >>> Why not QAPISchemaError, like for other semantic errors?
> >>>
> >>
> >> I think QAPISchemaError is a literal error of step 1, here
> >> it can't be used unless we record the text/line number belong to
> >> each expr.
> >
> > Reporting an error without a location is not nice!
> >
> > If decent error messages require recording locations, then we should
> > record locations.
> >
> > A real compiler frontend records full location information, i.e. every
> > node in the abstract syntax tree (or whatever else it produces) is
> > decorated with a location.
> >
> > Unfortunately, this wasn't done in qapi.py, so we get to retrofit it
> > now.
> >
> > Perhaps recording only locations of top-level expressions would suffice
> > to improve your error messages to acceptable levels. I'm not saying we
> > should take this shortcut, just pointing out it exists.
> >
> > qapi.py represents locations as character offset in the contents of the
> > schema file (QAPISchema.cursor), which it converts to line, column on
> > demand, in QAPISchemaError.__init__. If we keep things working that
> > way, the location data to record is the offset, not line, column.
> >
> >>>> +
> >>>> + discriminator_type = base_fields.get(discriminator)
> >>>> +
> >>>> + if not discriminator_type:
> >>>> + raise StandardError("Discriminator '%s' not found in schema\n"
> >>>> + % discriminator)
> >>>
> >>> Likewise.
> >>>
> >>>> +
> >>>> + return find_enum(discriminator_type)
> >>>
> >>> All errors should have a test in tests/qapi-schema/. I can try to add
> >>> tests for you when I rebase your 09/10.
> >>>
> >
>
