[ovs-dev] [PATCH 2/3] expr: Add and clarify a few comments and assertions.

Ben Pfaff blp at nicira.com
Thu Aug 27 05:33:51 UTC 2015


On Wed, Aug 26, 2015 at 04:21:42PM -0700, Andy Zhou wrote:
> A few questions in line:
> 
> On Tue, Aug 25, 2015 at 9:37 PM, Ben Pfaff <blp at nicira.com> wrote:
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> >  ovn/lib/expr.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
> > index 510a15e..20dd9c5 100644
> > --- a/ovn/lib/expr.c
> > +++ b/ovn/lib/expr.c
> > @@ -243,6 +243,11 @@ expr_fix_andor(struct expr *expr, bool short_circuit)
> >      }
> >  }
> >
> > +/* Returns 'expr' modified so that top-level oddities are fixed up:
> > + *
> > + *     - Eliminates any EXPR_T_BOOLEAN operands at the top level.
> > + *
> > + *     - Replaces one-operand EXPR_T_AND or EXPR_T_OR by its subexpression. */
> These comments seems to fit better with expr_fix_andor(). no?

expr_fix_andor() is just a helper for expr_fix(); only expr_fix() calls
it.

> >  static struct expr *
> >  expr_fix(struct expr *expr)
> >  {
> > @@ -626,6 +631,7 @@ make_cmp(struct expr_context *ctx,
> >
> >      if (f->symbol->level == EXPR_L_NOMINAL) {
> >          if (f->symbol->expansion) {
> > +            ovs_assert(f->symbol->width > 0);
> Is It the idea that string should have width==0 and exapnsion == NULL? If true,
> should expr_honors_invariants() also mention this?

No, it's that only subfields and predicates have expansions, and those
never have string type.  This was already implied, but I added the
assert to make it clear that the code just below that looks at the
'value' member of union expr_constant (instead of the 'string' member),
without checking for type, was correct.

> > -    /* Eliminate duplicates by sorting the subexpressions. */
> > +    /* Sort subexpressions by value and mask, to bring together duplicates. */
> Does this comment cover the string case?

The comment does, but the code is buggy for the string case; the next
commit fixes that.

> >      size_t n = list_size(&expr->andor);
> >      struct expr **subs = xmalloc(n * sizeof *subs);
> >



More information about the dev mailing list