[ovs-dev] [PATCH 2/4] expr: Refactor parsing of assignments and exchanges.

[Ben Pfaff]

On Tue, Jun 14, 2016 at 09:36:10AM -0500, Ryan Moats wrote:
> "dev" <dev-bounces at openvswitch.org> wrote on 06/09/2016 12:37:58 AM:
> 
> > From: Ben Pfaff <blp at ovn.org>
> > To: dev at openvswitch.org
> > Cc: Ben Pfaff <blp at ovn.org>
> > Subject: [ovs-dev] [PATCH 2/4] expr: Refactor parsing of assignments and
> exchanges.
> > Sent by: "dev" <dev-bounces at openvswitch.org>
> 
> [snip]
> 
> > --- a/ovn/lib/actions.c
> > +++ b/ovn/lib/actions.c
> > @@ -106,19 +106,35 @@ action_syntax_error(struct action_context *ctx,
> const char *message, ...)
> >  static void
> >  parse_set_action(struct action_context *ctx)
> >  {
> > -    struct expr *prereqs;
> > +    struct expr *prereqs = NULL;
> > +    struct expr_field dst;
> >      char *error;
> >
> > -    error = expr_parse_assignment(ctx->lexer, ctx->ap->symtab,
> > -                                  ctx->ap->lookup_port, ctx->ap->aux,
> > -                                  ctx->ofpacts, &prereqs);
> > +    error = expr_parse_field(ctx->lexer, ctx->ap->symtab, &dst);
> >      if (error) {
> > +        goto exit;
> > +    }
> > +
> > +    if (lexer_match(ctx->lexer, LEX_T_EXCHANGE)) {
> > +        error = expr_parse_exchange(ctx->lexer, &dst, ctx->ap->symtab,
> > +                                    ctx->ap->lookup_port, ctx->ap->aux,
> > +                                    ctx->ofpacts, &prereqs);
> > +    } else if (lexer_match(ctx->lexer, LEX_T_EQUALS)) {
> > +        error = expr_parse_assignment(
> > +            ctx->lexer, &dst, ctx->ap->symtab, ctx->ap->lookup_port,
> > +            ctx->ap->aux, ctx->ofpacts, &prereqs);
> > +    } else {
> > +        action_syntax_error(ctx, "expecting `=' or `<->'");
> > +    }
> > +
> > +exit:
> > +    if (!error) {
> > +        ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, prereqs);
> > +    } else {
> > +        expr_destroy(prereqs);
> >          action_error(ctx, "%s", error);
> >          free(error);
> > -        return;
> >      }
> > -
> > -    ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, prereqs);
> >  }
> >
> >  static void
> 
> I looked at this long and hard, and while I don't mind the use of gotos in
> C code,
> in this case I found it a bit "jarring" and wonder if the following isn't
> more readable:
> 
>     error = expr_parse_field(ctx->lexer, ctx->ap->symtab, &dst);
>     if (!error) {
>         if (lexer_match(ctx->lexer, LEX_T_EXCHANGE)) {
>             error = expr_parse_exchange(ctx->lexer, &dst, ctx->ap->symtab,
>                                         ctx->ap->lookup_port, ctx->ap->aux,
>                                         ctx->ofpacts, &prereqs);
>         } else if (lexer_match(ctx->lexer, LEX_T_EQUALS)) {
>             error = expr_parse_assignment(
>                 ctx->lexer, &dst, ctx->ap->symtab, ctx->ap->lookup_port,
>                 ctx->ap->aux, ctx->ofpacts, &prereqs);
>         } else {
>             action_syntax_error(ctx, "expecting `=' or `<->'");
>         }
>         if (!error) {
>             ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, prereqs);
>         }
>     }
>     if (error) {
>         expr_destroy(prereqs);
>         action_error(ctx, "%s", error);
>         free(error);
>     }
> 
> Ryan

Fair enough.  I made that change and applied this to master.