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. 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 hope that we can resolve this, technically and nontechnically, to everyone's satisfaction. Thanks Ben. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
