[1/3] qapi.py: Move common code to evaluate()

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(-)

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...)

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()?

def get_expr(fp):
    expr = ""

    for line in fp:
        if line.startswith('#') or line == '\n':
            continue
        if line.startswith(' '):
            expr += line
        elif expr:
            yield expr
            expr = line
        else:
            expr += line

    yield expr

def parse_schema(fp):
    exprs = []
    expr_eval

    for expr in get_expr(fp):
        expr_eval = evaluate(expr)
        if expr_eval.has_key('enum'):
            add_enum(expr_eval['enum'])
        ...
                
    return exprs

> 
>  def parse_schema(fp):
>      exprs = []
>      expr = ''
> -    expr_eval = None
> 
>      for line in fp:
>          if line.startswith('#') or line == '\n':
> @@ -90,23 +96,13 @@ def parse_schema(fp):
>          if line.startswith(' '):
>              expr += line
>          elif expr:
> -            expr_eval = evaluate(expr)
> -            if expr_eval.has_key('enum'):
> -                add_enum(expr_eval['enum'])
> -            elif expr_eval.has_key('union'):
> -                add_enum('%sKind' % expr_eval['union'])
> -            exprs.append(expr_eval)
> +            exprs.append(evaluate(expr))
>              expr = line
>          else:
>              expr += line
> 
>      if expr:
> -        expr_eval = evaluate(expr)
> -        if expr_eval.has_key('enum'):
> -            add_enum(expr_eval['enum'])
> -        elif expr_eval.has_key('union'):
> -            add_enum('%sKind' % expr_eval['union'])
> -        exprs.append(expr_eval)
> +        exprs.append(evaluate(expr))
> 
>      return exprs
> 
> -- 
> 1.8.1.4
> 
>

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.

Kevin

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
>