[ovs-dev,2/2] ofproto: Clean up style in ofproto_flow_mod_learn.

Submitted by Joe Stringer on March 17, 2017, 6:38 p.m.

Details

Message ID 20170317183835.26684-2-joe@ovn.org
State Accepted
Headers show

Commit Message

Joe Stringer March 17, 2017, 6:38 p.m.
It's slightly more cognitive load to read an inverse condition and then
invert it again to understand the 'else' condition.

Signed-off-by: Joe Stringer <joe@ovn.org>
---
 ofproto/ofproto.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Ben Pfaff March 17, 2017, 11:06 p.m.
On Fri, Mar 17, 2017 at 11:38:35AM -0700, Joe Stringer wrote:
> It's slightly more cognitive load to read an inverse condition and then
> invert it again to understand the 'else' condition.
> 
> Signed-off-by: Joe Stringer <joe@ovn.org>

It's also somewhat odd to handle the error condition before the common
case--I think that CodingStyle actually has something to say about
this--so maybe we should instead flip 'limited' to become its inverse
'below_limit' (and then rename the parameter to below_limitp or
below_limit_).

But if you like this better it's not a big deal to me, so:
Acked-by: Ben Pfaff <blp@ovn.org>
Joe Stringer March 21, 2017, 1:13 a.m.
On 17 March 2017 at 16:06, Ben Pfaff <blp@ovn.org> wrote:
> On Fri, Mar 17, 2017 at 11:38:35AM -0700, Joe Stringer wrote:
>> It's slightly more cognitive load to read an inverse condition and then
>> invert it again to understand the 'else' condition.
>>
>> Signed-off-by: Joe Stringer <joe@ovn.org>
>
> It's also somewhat odd to handle the error condition before the common
> case--I think that CodingStyle actually has something to say about
> this--so maybe we should instead flip 'limited' to become its inverse
> 'below_limit' (and then rename the parameter to below_limitp or
> below_limit_).
>
> But if you like this better it's not a big deal to me, so:
> Acked-by: Ben Pfaff <blp@ovn.org>

Thanks, I took your suggestion and applied to master.

Patch hide | download patch | download mbox

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index c6d83d4e6b29..e196597f4013 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -5118,15 +5118,15 @@  ofproto_flow_mod_learn(struct ofproto_flow_mod *ofm, bool keep_ref,
             rule_criteria_destroy(&criteria);
         }
 
-        if (!limited) {
+        if (limited) {
+            ofproto_flow_mod_uninit(ofm);
+        } else {
             ofm->version = rule->ofproto->tables_version + 1;
 
             error = ofproto_flow_mod_learn_start(ofm);
             if (!error) {
                 ofproto_flow_mod_learn_finish(ofm, NULL);
             }
-        } else {
-            ofproto_flow_mod_uninit(ofm);
         }
         ovs_mutex_unlock(&ofproto_mutex);