From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38855) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ur8Wh-0002uK-HF for qemu-devel@nongnu.org; Mon, 24 Jun 2013 11:18:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ur8Wg-0005t3-Ev for qemu-devel@nongnu.org; Mon, 24 Jun 2013 11:17:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58399) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ur8Wg-0005sq-7B for qemu-devel@nongnu.org; Mon, 24 Jun 2013 11:17:54 -0400 Date: Mon, 24 Jun 2013 17:17:35 +0200 From: Kevin Wolf Message-ID: <20130624151735.GH3338@dhcp-200-207.str.redhat.com> References: <1371659287-14331-1-git-send-email-kwolf@redhat.com> <1371659287-14331-2-git-send-email-kwolf@redhat.com> <20130621163957.GD12685@vm> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130621163957.GD12685@vm> Subject: Re: [Qemu-devel] [PATCH 1/3] qapi.py: Move common code to evaluate() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: mdroth Cc: lcapitulino@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, armbru@redhat.com 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 > > --- > > 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