[ovs-dev] [PATCH V2] feature: Create specific types for ofp and odp port

Alex Wang alexw at nicira.com
Wed Jun 12 00:03:46 UTC 2013


Thanks Ben,

The factors are all reasonable. Especially, the negation can be confusing.

Also, with you confirmation, I'll choose the "oft_to_u16()", "odp_to_u32()"
way. ;D

I'll adjust the code accordingly and resend a new version.

Kind Regards,
Alex Wang,


On Tue, Jun 11, 2013 at 4:55 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Mon, Jun 10, 2013 at 08:50:40AM -0700, Alex Wang wrote:
> > Subject: [PATCH] This patch implements "ofp_is_less_than()" and use it
> > to replace the PORT_COMPARE() function for port comparison.
> > Date: Mon, 10 Jun 2013 09:05:50 -0700
> > Message-Id: <1370880350-22000-1-git-send-email-alexw at nicira.com>
> > X-Mailer: git-send-email 1.7.9.5
> >
> > This patch implements "ofp_is_less_than()" and use it to replace the
> > PORT_COMPARE() function for port comparison.
> >
> > Signed-off-by: Alex Wang <alexw at nicira.com>
>
> I'm not a fan of this approach.  It's hard to put my finger on exactly
> why, but some factors that I don't like are:
>
>         * For me, anyway, it's hard to think through the ordering and
>           (sometimes) negation to figure out whether the condition is
>           correct.
>
>         * The form is different from a normal comparison, which makes
>           it a little harder to read.
>
>         * It's pretty long.
>
> I actually find the PORT_COMPARE form easier to read, although it
> definitely has downsides of its own.
>
> Here's another form that I find a little easier to read (which might
> only be because I've used it fairly often before):
>
>         static inline int
>         ofp_compare_3way(ofp_port_t a_, ofp_port_t b_)
>         {
>             uint32_t a = (OVS_FORCE uint32_t) a_;
>             uint32_t b = (OVS_FORCE uint32_t) b_;
>             return a < b ? -1 : a > b;
>         }
>
> Then you can write any condition you like by comparing against 0.  For
> example:
>         if (ofp_compare_3way(ofp_port, OFPP_MAX) >= 0)
> is equivalent to:
>         if (ofp_port >= OFPP_MAX)
> and
>         if (ofp_compare_3way(ofp_port, OFPP_MAX) < 0)
> is equivalent to:
>         if (ofp_port < OFPP_MAX)
>
> But I think I still like the way I proposed earlier better.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130611/5e7b3b5b/attachment-0003.html>


More information about the dev mailing list