Message ID | 20180206180049.7939-1-blp@ovn.org |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,1/3] expr: Fix some bad naming. | expand |
On Tue, Feb 6, 2018 at 11:30 PM, Ben Pfaff <blp@ovn.org> wrote: > expr_is_cmp() was badly named because it didn't just check for whether its > argument was an EXPR_T_CMP node. > > struct expr_sort's 'relop' member was badly named because it wasn't a > relational operator, it was a symbol. > > This commit improves both names. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > Acked-by: Numan Siddique <nusiddiq@redhat.com> Thanks Numan > --- > ovn/lib/expr.c | 46 ++++++++++++++++++++++++++-------------------- > 1 file changed, 26 insertions(+), 20 deletions(-) > > diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c > index 79ff45762f65..108af4a48210 100644 > --- a/ovn/lib/expr.c > +++ b/ovn/lib/expr.c > @@ -1922,8 +1922,13 @@ expr_simplify(struct expr *expr, > OVS_NOT_REACHED(); > } > > +/* Tests whether 'expr' is an expression over exactly one symbol: that is, > + * whether it is either a EXPR_T_CMP node or a tree of ANDs and ORs all > over > + * the same symbol. If it is, returns the symbol in question. If it is > not > + * (that is, if there is more than one symbol or no symbols at all), > returns > + * NULL. */ > static const struct expr_symbol * > -expr_is_cmp(const struct expr *expr) > +expr_get_unique_symbol(const struct expr *expr) > { > switch (expr->type) { > case EXPR_T_CMP: > @@ -1935,7 +1940,7 @@ expr_is_cmp(const struct expr *expr) > struct expr *sub; > > LIST_FOR_EACH (sub, node, &expr->andor) { > - const struct expr_symbol *symbol = expr_is_cmp(sub); > + const struct expr_symbol *symbol = > expr_get_unique_symbol(sub); > if (!symbol || (prev && symbol != prev)) { > return NULL; > } > @@ -1955,7 +1960,7 @@ expr_is_cmp(const struct expr *expr) > > struct expr_sort { > struct expr *expr; > - const struct expr_symbol *relop; > + const struct expr_symbol *symbol; > enum expr_type type; > }; > > @@ -1967,8 +1972,8 @@ compare_expr_sort(const void *a_, const void *b_) > > if (a->type != b->type) { > return a->type < b->type ? -1 : 1; > - } else if (a->relop) { > - int cmp = strcmp(a->relop->name, b->relop->name); > + } else if (a->symbol) { > + int cmp = strcmp(a->symbol->name, b->symbol->name); > if (cmp) { > return cmp; > } > @@ -2330,10 +2335,10 @@ crush_or(struct expr *expr, const struct > expr_symbol *symbol) > return expr_fix(expr); > } > > -/* Takes ownership of 'expr', which must be a cmp in the sense determined > by > - * 'expr_is_cmp(expr)', where 'symbol' is the symbol returned by that > function. > - * 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. */ > +/* Takes ownership of 'expr', which must have a unique symbol in the > sense of > + * 'expr_get_unique_symbol(expr)', where 'symbol' is the symbol returned > by > + * that function. 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. */ > static struct expr * > crush_cmps(struct expr *expr, const struct expr_symbol *symbol) > { > @@ -2372,8 +2377,8 @@ expr_sort(struct expr *expr) > i = 0; > LIST_FOR_EACH (sub, node, &expr->andor) { > subs[i].expr = sub; > - subs[i].relop = expr_is_cmp(sub); > - subs[i].type = subs[i].relop ? EXPR_T_CMP : sub->type; > + subs[i].symbol = expr_get_unique_symbol(sub); > + subs[i].type = subs[i].symbol ? EXPR_T_CMP : sub->type; > i++; > } > ovs_assert(i == n); > @@ -2382,17 +2387,17 @@ expr_sort(struct expr *expr) > > ovs_list_init(&expr->andor); > for (i = 0; i < n; ) { > - if (subs[i].relop) { > + if (subs[i].symbol) { > size_t j; > for (j = i + 1; j < n; j++) { > - if (subs[i].relop != subs[j].relop) { > + if (subs[i].symbol != subs[j].symbol) { > break; > } > } > > struct expr *crushed; > if (j == i + 1) { > - crushed = crush_cmps(subs[i].expr, subs[i].relop); > + crushed = crush_cmps(subs[i].expr, subs[i].symbol); > } else { > struct expr *combined = subs[i].expr; > for (size_t k = i + 1; k < j; k++) { > @@ -2400,7 +2405,7 @@ expr_sort(struct expr *expr) > subs[k].expr); > } > ovs_assert(!ovs_list_is_short(&combined->andor)); > - crushed = crush_cmps(combined, subs[i].relop); > + crushed = crush_cmps(combined, subs[i].symbol); > } > if (crushed->type == EXPR_T_BOOLEAN) { > if (!crushed->boolean) { > @@ -2472,7 +2477,7 @@ expr_normalize_and(struct expr *expr) > } > > ovs_assert(sub->type == EXPR_T_OR); > - const struct expr_symbol *symbol = expr_is_cmp(sub); > + const struct expr_symbol *symbol = expr_get_unique_symbol(sub); > if (!symbol || symbol->must_crossproduct) { > struct expr *or = expr_create_andor(EXPR_T_OR); > struct expr *k; > @@ -2842,7 +2847,7 @@ expr_to_matches(const struct expr *expr, > break; > > case EXPR_T_OR: > - if (expr_is_cmp(expr)) { > + if (expr_get_unique_symbol(expr)) { > struct expr *sub; > > LIST_FOR_EACH (sub, node, &expr->andor) { > @@ -2966,7 +2971,7 @@ expr_is_normalized_and(const struct expr *expr) > const struct expr *sub; > > LIST_FOR_EACH (sub, node, &expr->andor) { > - if (!expr_is_cmp(sub)) { > + if (!expr_get_unique_symbol(sub)) { > return false; > } > } > @@ -2986,11 +2991,12 @@ expr_is_normalized(const struct expr *expr) > return expr_is_normalized_and(expr); > > case EXPR_T_OR: > - if (!expr_is_cmp(expr)) { > + if (!expr_get_unique_symbol(expr)) { > const struct expr *sub; > > LIST_FOR_EACH (sub, node, &expr->andor) { > - if (!expr_is_cmp(sub) && !expr_is_normalized_and(sub)) { > + if (!expr_get_unique_symbol(sub) > + && !expr_is_normalized_and(sub)) { > return false; > } > } > -- > 2.15.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c index 79ff45762f65..108af4a48210 100644 --- a/ovn/lib/expr.c +++ b/ovn/lib/expr.c @@ -1922,8 +1922,13 @@ expr_simplify(struct expr *expr, OVS_NOT_REACHED(); } +/* Tests whether 'expr' is an expression over exactly one symbol: that is, + * whether it is either a EXPR_T_CMP node or a tree of ANDs and ORs all over + * the same symbol. If it is, returns the symbol in question. If it is not + * (that is, if there is more than one symbol or no symbols at all), returns + * NULL. */ static const struct expr_symbol * -expr_is_cmp(const struct expr *expr) +expr_get_unique_symbol(const struct expr *expr) { switch (expr->type) { case EXPR_T_CMP: @@ -1935,7 +1940,7 @@ expr_is_cmp(const struct expr *expr) struct expr *sub; LIST_FOR_EACH (sub, node, &expr->andor) { - const struct expr_symbol *symbol = expr_is_cmp(sub); + const struct expr_symbol *symbol = expr_get_unique_symbol(sub); if (!symbol || (prev && symbol != prev)) { return NULL; } @@ -1955,7 +1960,7 @@ expr_is_cmp(const struct expr *expr) struct expr_sort { struct expr *expr; - const struct expr_symbol *relop; + const struct expr_symbol *symbol; enum expr_type type; }; @@ -1967,8 +1972,8 @@ compare_expr_sort(const void *a_, const void *b_) if (a->type != b->type) { return a->type < b->type ? -1 : 1; - } else if (a->relop) { - int cmp = strcmp(a->relop->name, b->relop->name); + } else if (a->symbol) { + int cmp = strcmp(a->symbol->name, b->symbol->name); if (cmp) { return cmp; } @@ -2330,10 +2335,10 @@ crush_or(struct expr *expr, const struct expr_symbol *symbol) return expr_fix(expr); } -/* Takes ownership of 'expr', which must be a cmp in the sense determined by - * 'expr_is_cmp(expr)', where 'symbol' is the symbol returned by that function. - * 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. */ +/* Takes ownership of 'expr', which must have a unique symbol in the sense of + * 'expr_get_unique_symbol(expr)', where 'symbol' is the symbol returned by + * that function. 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. */ static struct expr * crush_cmps(struct expr *expr, const struct expr_symbol *symbol) { @@ -2372,8 +2377,8 @@ expr_sort(struct expr *expr) i = 0; LIST_FOR_EACH (sub, node, &expr->andor) { subs[i].expr = sub; - subs[i].relop = expr_is_cmp(sub); - subs[i].type = subs[i].relop ? EXPR_T_CMP : sub->type; + subs[i].symbol = expr_get_unique_symbol(sub); + subs[i].type = subs[i].symbol ? EXPR_T_CMP : sub->type; i++; } ovs_assert(i == n); @@ -2382,17 +2387,17 @@ expr_sort(struct expr *expr) ovs_list_init(&expr->andor); for (i = 0; i < n; ) { - if (subs[i].relop) { + if (subs[i].symbol) { size_t j; for (j = i + 1; j < n; j++) { - if (subs[i].relop != subs[j].relop) { + if (subs[i].symbol != subs[j].symbol) { break; } } struct expr *crushed; if (j == i + 1) { - crushed = crush_cmps(subs[i].expr, subs[i].relop); + crushed = crush_cmps(subs[i].expr, subs[i].symbol); } else { struct expr *combined = subs[i].expr; for (size_t k = i + 1; k < j; k++) { @@ -2400,7 +2405,7 @@ expr_sort(struct expr *expr) subs[k].expr); } ovs_assert(!ovs_list_is_short(&combined->andor)); - crushed = crush_cmps(combined, subs[i].relop); + crushed = crush_cmps(combined, subs[i].symbol); } if (crushed->type == EXPR_T_BOOLEAN) { if (!crushed->boolean) { @@ -2472,7 +2477,7 @@ expr_normalize_and(struct expr *expr) } ovs_assert(sub->type == EXPR_T_OR); - const struct expr_symbol *symbol = expr_is_cmp(sub); + const struct expr_symbol *symbol = expr_get_unique_symbol(sub); if (!symbol || symbol->must_crossproduct) { struct expr *or = expr_create_andor(EXPR_T_OR); struct expr *k; @@ -2842,7 +2847,7 @@ expr_to_matches(const struct expr *expr, break; case EXPR_T_OR: - if (expr_is_cmp(expr)) { + if (expr_get_unique_symbol(expr)) { struct expr *sub; LIST_FOR_EACH (sub, node, &expr->andor) { @@ -2966,7 +2971,7 @@ expr_is_normalized_and(const struct expr *expr) const struct expr *sub; LIST_FOR_EACH (sub, node, &expr->andor) { - if (!expr_is_cmp(sub)) { + if (!expr_get_unique_symbol(sub)) { return false; } } @@ -2986,11 +2991,12 @@ expr_is_normalized(const struct expr *expr) return expr_is_normalized_and(expr); case EXPR_T_OR: - if (!expr_is_cmp(expr)) { + if (!expr_get_unique_symbol(expr)) { const struct expr *sub; LIST_FOR_EACH (sub, node, &expr->andor) { - if (!expr_is_cmp(sub) && !expr_is_normalized_and(sub)) { + if (!expr_get_unique_symbol(sub) + && !expr_is_normalized_and(sub)) { return false; } }
expr_is_cmp() was badly named because it didn't just check for whether its argument was an EXPR_T_CMP node. struct expr_sort's 'relop' member was badly named because it wasn't a relational operator, it was a symbol. This commit improves both names. Signed-off-by: Ben Pfaff <blp@ovn.org> --- ovn/lib/expr.c | 46 ++++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 20 deletions(-)