Re: [Qemu-devel] [PATCH 1/3] qapi.py: Move common code to evaluate()

[mdroth <mdroth@linux.vnet.ibm.com>]

On Mon, Jun 24, 2013 at 05:17:35PM +0200, Kevin Wolf wrote:
> Am 21.06.2013 um 18:39 hat mdroth geschrieben:
> > On Wed, Jun 19, 2013 at 06:28:05PM +0200, Kevin Wolf wrote:
> > > Don't duplicate more code than is really necessary.
> > > 
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  scripts/qapi.py | 24 ++++++++++--------------
> > >  1 file changed, 10 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > > index 02ad668..3a64769 100644
> > > --- a/scripts/qapi.py
> > > +++ b/scripts/qapi.py
> > > @@ -76,12 +76,18 @@ def parse(tokens):
> > >          return tokens[0], tokens[1:]
> > > 
> > >  def evaluate(string):
> > > -    return parse(map(lambda x: x, tokenize(string)))[0]
> > > +    expr_eval = parse(map(lambda x: x, tokenize(string)))[0]
> > > +
> > > +    if expr_eval.has_key('enum'):
> > > +        add_enum(expr_eval['enum'])
> > > +    elif expr_eval.has_key('union'):
> > > +        add_enum('%sKind' % expr_eval['union'])
> > > +
> > > +    return expr_eval
> > 
> > add_enum() adds stuff to a global table, so I don't really like the idea
> > of pushing it further down the stack (however inconsequential it may be
> > in this case...)
> 
> As if it made any difference if it's one level more or less... It's
> already buried in the "library".
> 
> > I think the real reason we have repetition is the extra 'flush' we do
> > after the for loop below to handle the last expression we read from a
> > schema, which leads to a repeat of one of the clauses in the loop body.
> > I've wanted to get rid of it a few times in the past so this is probably
> > a good opportunity to do so.
> > 
> > Could you try adapting something like the following to keep the
> > global stuff in parse_schema()?
> 
> I don't think there's any value in keeping the global stuff in
> parse_schema() (or, to be precise, in functions directly called by
> parse_schema()) , and I found it quite nice to have evaluate()
> actually evaluate something instead of just parsing it.
> 
> But I agree that duplicating code, as small as it may be now, isn't
> nice, so I can try to use the get_expr() thing. I just would prefer to
> move the evaluation to evaluate() anyway.

evaluate() is basically the qapi version of python's eval(), but with
specially handling for JSON objects that retains the original ordering
of the keys by mapping JSON strings to OrderedDicts instead of dicts. If
it weren't for that requirement we'd be call eval() in place of
evaluate() and drop all our awesome parsing/tokenizing code.

It's still not a huge deal, but given an alternative that's also
beneficial in other ways I think we should avoid it.

> 
> Kevin
>