Message ID | 1371659287-14331-2-git-send-email-kwolf@redhat.com |
---|---|
State | New |
Headers | show |
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 >
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 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
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(-)