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 <b...@nicira.com> wrote:
> > Signed-off-by: Ben Pfaff <b...@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);
> >
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
