> On Sep 30, 2015, at 1:56 PM, Ben Pfaff <[email protected]> wrote:
>
> static bool
> -expand_symbol(struct expr_context *ctx, struct expr_field *f,
> - struct expr **prereqsp)
> +expand_symbol(struct expr_context *ctx, bool rw, bool swap,
> + struct expr_field *f, struct expr **prereqsp)
I assume "rw" is "read/write". It might be nice to document it, though.
> {
> + const struct expr_symbol *orig_symbol = f->symbol;
> +
> if (f->symbol->expansion && f->symbol->level != EXPR_L_ORDINAL) {
> - expr_error(ctx, "Predicate symbol %s cannot be used in assignment.",
> - f->symbol->name);
> + expr_error(ctx, "Predicate symbol %s cannot be used in %s.",
> + f->symbol->name, swap ? "exchange" : "assignment");
In most internal uses, "swap" is used instead of "exchange"; but external uses
seem to prefer "exchange". It's not a big deal, but it can be a bit easier to
work through the code if the names are the same.
> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> index 28b0d2c..3ae99b3 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -806,11 +806,11 @@
> <p>
> Sets data or metadata field <var>field1</var> to the value of data
> or metadata field <var>field2</var>, e.g. <code>reg0 =
> - ip4.src;</code> to copy <code>ip4.src</code> into
> - <code>reg0</code>. To modify only a subset of a field's bits,
> - specify a subfield for <var>field1</var> or <var>field2</var> or
> - both, e.g. <code>vlan.pcp = reg0[0..2];</code> to set the VLAN
> PCP
> - from the least-significant bits of <code>reg0</code>.
> + ip4.src;</code> copies <code>ip4.src</code> into
> <code>reg0</code>.
> + To modify only a subset of a field's bits, specify a subfield for
> + <var>field1</var> or <var>field2</var> or both, e.g.
> <code>vlan.pcp
> + = reg0[0..2];</code> copies the least-significant bits of
> + <code>reg0</code> into the VLAN PCP.
This isn't a big deal, but it seems like it belongs more in the previous patch.
> -(){}[[]]==!=<<=>>=!&&||..,;= => ( ) { } [[ ]] == != < <= > >= ! && || .. , ;
> =
> +(){}[[]]==!=<<=>>=!&&||..,;=<-> => ( ) { } [[ ]] == != < <= > >= ! && || ..
> , ; = <->
> & => error("`&' is only valid as part of `&&'.")
> | => error("`|' is only valid as part of `||'.")
Not related to this patch in particular, but it might be worth adding a comment
about this test, since it looks odd.
> +ip.proto = reg0[0..7]; => Field ip.proto is not modifiable.
Also not a big deal, but it seems like this test belongs in the previous patch.
It's nice to see push/pop being put to use!
Acked-by: Justin Pettit <[email protected]>
--Justin
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
