On Tue, Mar 20, 2018 at 09:35:17AM +0000, Weglicki, MichalX wrote: > > -----Original Message----- > > From: Ilya Maximets [mailto:[email protected]] > > Sent: Tuesday, March 20, 2018 9:50 AM > > To: Weglicki, MichalX <[email protected]>; [email protected] > > Cc: Heetae Ahn <[email protected]>; Ben Pfaff <[email protected]>; > > Stokes, Ian <[email protected]> > > Subject: Re: [PATCH] netdev-dpdk: Refactor custom stats. > > > > On 19.03.2018 13:22, Weglicki, MichalX wrote: > > > Hello, > > > > Hello. > > > > > > > > I've went through the patch quite carefully. > > > > Thanks for reviewing this. > > > > > Main change refactors creation cached IDs and Names from IF-like code > > > block to "Goto" code block. > > > > Current code is over-nested. It has nesting level of 6 (7 including > > function definition). > > It's like: > > netdev_dpdk_configure_xstats > > { > > if () { > > if () { > > ... > > } else { > > ... > > if () { > > ... > > if () { > > } else if { > > } > > ... > > if () { > > for () { > > if () { <-- level #7 ! > > } > > } > > } else { > > } > > } > > } > > } else { > > } > > if () { > > } > > } > > > > > > IMHO, This is not readable. Especially, combining with constant line wraps > > because > > of limited line lengths. I wanted to make plain execution sequence to > > simplify > > understanding of the code. Most of the 'if' statements are just error > > checking. > > And the single exit point from such conditions usually implemented by > > 'goto's. > > It's a common practice for system programming. It doesn't worth to keep so > > deep > > nesting level for error checking conditions. This also matches the style of > > all > > the other code in netdev-dpdk and not only here. > > For me it is straightforward error checking, so I don't really see an > advantage. > Not everything is implemented using "goto" in netdev-dpdk. I assume this is > preference thing so maybe we could ask someone to make a call, Ben/Ian?
When there's deep nesting, "goto" is often easier to read. This looks like one of those cases. Sometimes, a third choice is to use a wrapper function that does the deallocation (or other resource freeing) and then substitute "return" for "goto". _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
