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

[Darrell Ball]

On 11/19/17, 11:18 AM, "Ben Pfaff" <blp at ovn.org> wrote:

    On Fri, Nov 17, 2017 at 07:34:49PM +0000, Darrell Ball wrote:
    > This patch mostly breaks OVS coding style in many instances and also invents its own coding guidelines.
    > 
    > 1/OVS prefers variable declarations near their usage and your patch violates extensively.
    > Most of this patch is related to this.
    > I’ll point out a few to provide examples, but I don’t like this – nack.
    
    This reads to me as more aggressive than necessary.  The OVS coding
    style says to mix declarations and code within a block "judiciously" and
    to "keep declarations nicely grouped together in the beginning of a
    block if possible."  I think that this now inaccurately describes
    current coding practice in OVS, as well as best practice for modern C,
    but it is still what the document says, so it is understandable that
    anyone reading the document would want to fix the code to match.


    I tend to take this patch as a kind of bug report against the coding
    style document.  The right response to that might be to explain why the
    coding style document is wrong and the history behind it, and to thank
    the submitter (thank you, Flavio!).  I suggest that we should update the
    coding style document to match our current practice (and perhaps to
    motivate current practice with rationale).
    
    > 2/In many instances where this patch moves “&&” at beginning of next line rather than at end of line
    > There is no coding style for this and different OVS code uses both styles.
    > I prefer to have the operator at the end of line as it makes it clear there is a continuation.
    
    This reads as somewhat aggressive too, especially following #1.

Hmm, not intentionally then “and it's certainly not ad hominem” – I read that on another alias thread once
(it is easy to search for, btw) - “not ad hominem” is accurate here as well.

    
    I believe that Flavio interpreted the examples in the coding style
    document as normative.  That is understandable, because the document
    does not say anything explicitly.  When we wrote the document in 2007, I
    don't remember Justin and I even discussing whether lines should be
    split before or after binary operators, and we've never (as far as I
    know) interpreted those particular examples as normative in that aspect.
    
    Again, I would prefer to interpret this part of the patch as a bug
    report against the coding standards.  Thank you, Flavio, for bringing
    this to our attention.  I think that it would be a good idea to modify
    at least one of the examples to show a line break after a binary
    operator, and perhaps to add an explicit statement that there is no norm
    for positioning binary operators before or after a line break.  (Maybe
    there is someone out there who wants to campaign for particular
    positioning, but I'll leave that to whoever that is.)
    
    > There is a missing space after “}” in one instance - thanks
    > There are also use of full 79 line lengths in a few places, which looked ok, but I did not check carefully.
    > There is some missed extra newlines after declarations, which generally looks ok; I’ll check more however.
    > I also see some extra newlines removed which looked ok, but I’ll check more.
    > 
    > I’ll submit my own patch since I don’t agree with “1” and “2”.
    
    Sometimes it is helpful to propose a competing patch, but usually that
    would follow something closer to coming to consensus, or sometimes after
    discussion makes it clear that the new proposer understands some issue
    better than the original poster or is otherwise better situated to
    help.  In this case, it reads more as a kind of aggression.

I was not referring to a competing patch, but rather a patch that fixes the code to match 
the effective coding standard, as I understand it, in some missing cases, but also has a commit message
that lists each coding standard. The patch is different than Flavio’s but Flavio’s patch made me
look more while I was working on related patches.

https://patchwork.ozlabs.org/patch/839358/


    
    I hope that we can resolve this, technically and nontechnically, to
    everyone's satisfaction.
    
    Thanks
    
    Ben.