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

Andy Zhou azhou at nicira.com
Wed Aug 26 23:21:42 UTC 2015


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?
>  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?
>              for (size_t i = 0; i < cs->n_values; i++) {
>                  const union mf_subvalue *value = &cs->values[i].value;
>                  bool positive = (value->integer & htonll(1)) != 0;
> @@ -1806,13 +1812,15 @@ compare_expr(const void *a_, const void *b_)
>      return d;
>  }
>
> +/* Implementation of crush_cmps() for expr->type == EXPR_T_OR. */
>  static struct expr *
>  crush_or(struct expr *expr, const struct expr_symbol *symbol)
>  {
>      struct expr *sub, *next = NULL;
>
>      /* First, crush all the subexpressions.  That might eliminate the
> -     * OR-expression entirely; if so, return the result. */
> +     * OR-expression entirely; if so, return the result.  Otherwise, 'expr'
> +     * is now a disjunction of cmps over the same symbol. */
>      LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) {
>          list_remove(&sub->node);
>          expr_insert_andor(expr, next, crush_cmps(sub, symbol));
> @@ -1822,7 +1830,7 @@ crush_or(struct expr *expr, const struct expr_symbol *symbol)
>          return expr;
>      }
>
> -    /* Eliminate duplicates by sorting the subexpressions. */
> +    /* Sort subexpressions by value and mask, to bring together duplicates. */
Does this comment cover the string case?
>      size_t n = list_size(&expr->andor);
>      struct expr **subs = xmalloc(n * sizeof *subs);
>
> @@ -1850,8 +1858,11 @@ crush_or(struct expr *expr, const struct expr_symbol *symbol)
>      return expr_fix(expr);
>  }
>
> -/* Converts 'expr', which must be a cmp in the sense determined by
> - * expr_is_cmp().  Returns a cmp, a disjunction of cmps, or a boolean. */
> +/* Takes ownership of 'expr', which must be a cmp in the sense determined by
> + * expr_is_cmp(), and returns an equivalent expression owned by the caller that
> + * is a single EXPR_T_CMP or a disjunction of them or a EXPR_T_BOOLEAN.
> + *
> + * 'symbol' must be expr_is_cmp(expr). */
>  static struct expr *
>  crush_cmps(struct expr *expr, const struct expr_symbol *symbol)
>  {
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list