[ovs-dev] [PATCH] conntrack: coding style fixes.

[Flavio Leitner]

Hi Ben,

On Sun, 19 Nov 2017 10:58:11 -0800
Ben Pfaff <blp at ovn.org> wrote:

> On Fri, Nov 17, 2017 at 03:51:34PM -0200, Flavio Leitner wrote:
> > No functional change, just fixing coding style.
> > 
> > Signed-off-by: Flavio Leitner <fbl at sysclose.org>  
> 
> I see from the thread that there's some controversy here.  I think that
> there are two aspects to it: technical and nontechnical.  Please allow
> me address them separately, starting with the technical side.
> 
> The code that later evolved to become OVS originated in 2007 at Nicira.
> At the time, support for C99 features was far from universal among the
> compilers that seemed potentially important.  Justin and I wrote the
> initial coding style document to emphasize portability.  Therefore,
> originally it forbid any mid-block declarations as well as other
> features with limited portability (e.g. C++ style // comments).
> 
> Since then there have been two relevant changes.  First, C99 is becoming
> more pervasive across the compiler world.  Second, it's become clear
> that OVS really only needs to care about three particular compilers,
> which are GCC, Clang, and MSVC, and all of these compilers now support
> C99 (and an increasing fraction of C11).
> 
> As these changes have occurred, we have been evolving the OVS coding
> style too.  Sometimes, the evolution has happened first in the coding
> style document (and later migrated to coding practice), and other times,
> it has happened in the code itself (and later migrated to the coding
> style document).  I think that this slow co-evolution is natural, since
> it's rare for someone to go through all the code at once and update the
> style to match changes to the coding style document.
> 
> Sometimes, OVS has only half-embraced new coding style features enabled
> by compiler advancements.  Mid-block declarations are an example.  I've
> been writing C code since 1992 or so (and I hope I'm getting better at
> it!) but I've only been able to rely on portable support for mid-block
> declarations for a few years.  For better or for worse, when in 2014
> Jarno proposed that we allow mid-block declarations in OVS, the fossil
> in me, not used to seeing mid-block declarations in C, made me
> reluctant.  You can see that, originally, Jarno proposed simply allowing
> mid-block declarations in his message
> https://mail.openvswitch.org/pipermail/ovs-dev/2014-May/284130.html.  In
> a reply, Ethan and I were the one who pushed back, with my "I'm still
> really nervous about this one (I think it often uglifies code)..." and
> Ethan's "...The reason is that most of the code doesn't do these things,
> and there's a value in consistency."
> 
> Since 2014, I feel that the balance has shifted.  We have increased the
> amount of code that uses mid-block declarations.  My increasing
> familiarity with the style has reduced my misgivings, and even caused me
> to embrace them: I really like the reduced potential for use of
> uninitialized data by being able, far more often, to declare and
> initialize a variable at the same time.  Most importantly for me
> personally, this save time in code review, since I spend less time
> flicking my eyes and scrolling my window back and forth between
> declaration and use to reassure myself that initialization precedes use.
> I still see some potential for ugliness, but the practical virtues of
> mid-block declarations usually outweigh them.
> 
> What I'm saying here is, "I was wrong."  I honestly forgot we had
> anything in the coding style document that discouraged mid-block
> declarations.  I like consistency between code and documentation but I
> believe that the right thing to do here is to update the coding style
> document, by removing anything that discourages mid-block declarations.
> Now I use them pervasively and I want to encourages others to do so too.
> 
> That's my long, rambling defense of mid-block declarations.  The other
> technical issue here is about whether && and || should be at the
> beginning or end of a line.  The coding style document, intentionally,
> does not take an explicit position on this, and I did not realize that
> the examples in the document are biased toward putting them at the
> beginning of the line.  OVS uses a mix of positions.  I personally tend
> to put them at the beginning of a line because I have a long history of
> writing code in the GNU coding style, which mandates this positioning,
> but others feel exactly the opposite (for example I believe that kernel
> style favors end-of-line positioning).  This is a debate that frankly I
> don't care much for and I'd prefer to leave it as a matter to individual
> taste.  (This is also why the coding style document gives some freedom
> in the positioning of /* and */ in comment blocks: there is a range of
> viewpoints on this and I think the answer doesn't matter.)  If
> necessary, I favor adding an explicit statement to the document saying
> that lines may be split before or after binary operators.  (On the other
> hand I do prefer ? and : at the beginning of a line.)

It all makes sense except for the freedom in the coding style. Too much
freedom means no standard.

It is natural that the code evolves and maybe new things will impact on
the current coding style guidelines.  I agree we don't need to rewrite
OVS every time that happens, but as it happened in this case, one could
update the style being used in a file at some point, so that the coding
style document rules the major portion of the code base.

I came from the kernel and yet the patch uses the other style, which
I figured was the preferable one. Even with that fossil in me a bit
reluctant to adopt mid-block declarations, I honestly don't care as
long as I can see a pattern ruling a good portion of the code.

Yes, I realize the proposed patch (please drop it) has changes going
in the other direction because it never occurred to me that we could
update the coding style with regards to where to break lines. 

How to decide which one is preferable?  Maybe take a snapshot of OVS
today and see what is mostly used and take from there?

Thanks,
-- 
Flavio