warning: massive change to conditional coding style in net?

warning: massive change to conditional coding style in net?

[William Allen Simpson]

Over the past several days, David Miller (with help from Joe Perches)
made sweeping changes to the format of conditional statements in the
net tree -- the equivalent of mass patches that change spaces.

This makes writing patches for multiple versions of the tree very
difficult, and will make future pullups problematic.  It's enough to
make a grown man cry....  Patching conflicts everywhere!

CodingStyle is mute on this issue.  Does Linus have a preference?

My personal practice (based on decades of open source projects) has
been to use a form already used in the same file or section of code,
matching the existing practice.

If this is to be done everywhere, CodingStyle (and SubmittingPatches)
should be updated.

Currently, roughly 19% (7855 lines) of the -2.6 tree uses leading form:

	if (condition
	    && condition
	    && (condition
	        || condition
	        || condition)) {

Single spaced is also fairly common:

	if (condition
	 && condition
	 && (condition
	  || condition
	  || condition)) {

The advantage of the leading form is *readability* due to indentation,
ease of patching and reading patches (changes affect only 1 line,
instead of previous and following lines), and especially conditionals
within #if sections.  Also, shorter lines (by 3 characters).

The other 81% uses trailing form, often with odd random line breaks:

	if (condition &&
             condition && (condition || condition ||
	    condition)) {

Miller (with Perches) changed hundreds (thousands?) of these to
trailing form.  This results in a number of hilarious examples --
lines with both leading and trailing, lines with only &&, etc.  A
small sample for illustration:

===

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 322b408..b78e615 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -264,9 +264,11 @@ int ip_mc_output(struct sk_buff *skb)

  		   This check is duplicated in ip_mr_input at the moment.
  		 */
-		    && ((rt->rt_flags&RTCF_LOCAL) || !(IPCB(skb)->flags&IPSKB_FORWARDED))
+		    &&
+		    ((rt->rt_flags & RTCF_LOCAL) ||
+		     !(IPCB(skb)->flags & IPSKB_FORWARDED))
  #endif
-		) {
+		   ) {
  			struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC);
  			if (newskb)
  				NF_HOOK(PF_INET, NF_INET_POST_ROUTING, newskb,
diff --git a/net/irda/irnet/irnet_irda.c b/net/irda/irnet/irnet_irda.c
index cccc2e9..b26dee7 100644
--- a/net/irda/irnet/irnet_irda.c
+++ b/net/irda/irnet/irnet_irda.c
@@ -1403,8 +1403,8 @@ irnet_connect_indication(void *		instance,
    /* Socket already connecting ? On primary ? */
    if(0
  #ifdef ALLOW_SIMULT_CONNECT
-     || ((irttp_is_primary(server->tsap) == 1)	/* primary */
-	 && (test_and_clear_bit(0, &new->ttp_connect)))
+     || ((irttp_is_primary(server->tsap) == 1) &&	/* primary */
+	 (test_and_clear_bit(0, &new->ttp_connect)))
  #endif /* ALLOW_SIMULT_CONNECT */
       )
      {
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index 7034ea4..dd9414e 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -170,21 +170,23 @@ restart:
  	for (s = sht[h1]; s; s = s->next) {
  		if (dst[RSVP_DST_LEN-1] == s->dst[RSVP_DST_LEN-1] &&
  		    protocol == s->protocol &&
-		    !(s->dpi.mask & (*(u32*)(xprt+s->dpi.offset)^s->dpi.key))
+		    !(s->dpi.mask &
+		      (*(u32*)(xprt+s->dpi.offset)^s->dpi.key)) &&
  #if RSVP_DST_LEN == 4
-		    && dst[0] == s->dst[0]
-		    && dst[1] == s->dst[1]
-		    && dst[2] == s->dst[2]
+		    dst[0] == s->dst[0] &&
+		    dst[1] == s->dst[1] &&
+		    dst[2] == s->dst[2] &&
  #endif
-		    && tunnelid == s->tunnelid) {
+		    tunnelid == s->tunnelid) {

  			for (f = s->ht[h2]; f; f = f->next) {
  				if (src[RSVP_DST_LEN-1] == f->src[RSVP_DST_LEN-1] &&
  				    !(f->spi.mask & (*(u32*)(xprt+f->spi.offset)^f->spi.key))
  #if RSVP_DST_LEN == 4
-				    && src[0] == f->src[0]
-				    && src[1] == f->src[1]
-				    && src[2] == f->src[2]
+				    &&
+				    src[0] == f->src[0] &&
+				    src[1] == f->src[1] &&
+				    src[2] == f->src[2]
  #endif
  				    ) {
  					*res = f->res;

Re: warning: massive change to conditional coding style in net?

[Jarek Poplawski]

On 30-11-2009 11:36, William Allen Simpson wrote:
> Over the past several days, David Miller (with help from Joe Perches)
> made sweeping changes to the format of conditional statements in the
> net tree -- the equivalent of mass patches that change spaces.
> 
> This makes writing patches for multiple versions of the tree very
> difficult, and will make future pullups problematic.  It's enough to
> make a grown man cry....  Patching conflicts everywhere!
> 
> CodingStyle is mute on this issue.  Does Linus have a preference?
> 
> My personal practice (based on decades of open source projects) has
> been to use a form already used in the same file or section of code,
> matching the existing practice.
> 
> If this is to be done everywhere, CodingStyle (and SubmittingPatches)
> should be updated.
> 
> Currently, roughly 19% (7855 lines) of the -2.6 tree uses leading form:
> 
> 	if (condition
> 	    && condition
> 	    && (condition
> 	        || condition
> 	        || condition)) {
> 
> Single spaced is also fairly common:
> 
> 	if (condition
> 	 && condition
> 	 && (condition
> 	  || condition
> 	  || condition)) {
> 
> The advantage of the leading form is *readability* due to indentation,
> ease of patching and reading patches (changes affect only 1 line,
> instead of previous and following lines), and especially conditionals
> within #if sections.  Also, shorter lines (by 3 characters).
> 
> The other 81% uses trailing form, often with odd random line breaks:
> 
> 	if (condition &&
>              condition && (condition || condition ||
> 	    condition)) {
> 
> Miller (with Perches) changed hundreds (thousands?) of these to
> trailing form.  This results in a number of hilarious examples --
> lines with both leading and trailing, lines with only &&, etc.  A
> small sample for illustration:

Yes, it's even enough to make a grown man laugh....

Jarek P.

Re: warning: massive change to conditional coding style in net?

[Alan Cox]

> > Miller (with Perches) changed hundreds (thousands?) of these to
> > trailing form.  This results in a number of hilarious examples --
> > lines with both leading and trailing, lines with only &&, etc.  A
> > small sample for illustration:
> 
> Yes, it's even enough to make a grown man laugh....

IMHO for left to right languages dangly bits want to be on the right,
because that is where your eyes are when you hit the end of the previous
line, so the dangly bit is already in your line of vision and probably
even in focus.

Re: warning: massive change to conditional coding style in net?

[Jarek Poplawski]

Alan Cox wrote, On 11/30/2009 02:54 PM:

>>> Miller (with Perches) changed hundreds (thousands?) of these to
>>> trailing form.  This results in a number of hilarious examples --
>>> lines with both leading and trailing, lines with only &&, etc.  A
>>> small sample for illustration:
>> Yes, it's even enough to make a grown man laugh....
> 
> IMHO for left to right languages dangly bits want to be on the right,
> because that is where your eyes are when you hit the end of the previous
> line, so the dangly bit is already in your line of vision and probably
> even in focus.

Of course this is right. Plus "IMHO" "habit is the second nature", so
riding the same side of the road helps, even if it's the wrong side. ;-)

Jarek P.

Re: warning: massive change to conditional coding style in net?

[Joe Perches]

On Mon, 2009-11-30 at 05:36 -0500, William Allen Simpson wrote:
> Over the past several days, David Miller (with help from Joe Perches)
> made sweeping changes to the format of conditional statements in the
> net tree -- the equivalent of mass patches that change spaces.
> This makes writing patches for multiple versions of the tree very
> difficult, and will make future pullups problematic.

If it makes getting tcp cookies accepted difficult,
a reversion is simple.  That style isn't as important.

I think writing a single set of patches for multiple
versions of linux is not feasible.  Feature changes
occur in kernel source daily.

> 	if (condition
> 	    && condition
> 	    && (condition
> 	        || condition
> 	        || condition)) {

The above is my personally preferred style.

> 	if (condition &&
>              condition && (condition || condition ||
> 	    condition)) {

Except for the odd spacing, this is the significant
majority of net/ style.

The leading style was < 10%.  It's less now.

> Miller (with Perches) changed hundreds (thousands?) of these to
> trailing form.  This results in a number of hilarious examples --
> lines with both leading and trailing, lines with only &&, etc.

Nearly all existing.

Re: warning: massive change to conditional coding style in net?

[William Allen Simpson]

Joe Perches wrote:
> On Mon, 2009-11-30 at 05:36 -0500, William Allen Simpson wrote:
>> Over the past several days, David Miller (with help from Joe Perches)
>> made sweeping changes to the format of conditional statements in the
>> net tree -- the equivalent of mass patches that change spaces.
>> This makes writing patches for multiple versions of the tree very
>> difficult, and will make future pullups problematic.
> 
> If it makes getting tcp cookies accepted difficult,
> a reversion is simple.  That style isn't as important.
> 
Then why make an *un*important (yet sweeping) change?


> I think writing a single set of patches for multiple
> versions of linux is not feasible.  Feature changes
> occur in kernel source daily.
> 
My patches were carefully written and applied with small fuzz to .30,
and .31, and .32-rc3.


>> 	if (condition
>> 	    && condition
>> 	    && (condition
>> 	        || condition
>> 	        || condition)) {
> 
> The above is my personally preferred style.
> 
That seems fine to me.  And in some areas of the tree, nearly 100% of
other contributors, too.

My personally preferred style is the single spaced variant, that also
conforms to the strict letter of CodingStyle, to wit:

   Use one space around (on each side of) most binary and ternary operators


>> 	if (condition &&
>>              condition && (condition || condition ||
>> 	    condition)) {
> 
> Except for the odd spacing, this is the significant
> majority of net/ style.
> 
> The leading style was < 10%.  It's less now.
> 
That's only true in net/ -- since the overall tree was 18.7%, with
net/ < 10%, the density was *much* higher elsewhere.

But more important, at least to my thinking, is keeping patches simple
by conforming to the *existing* style in the section of code.  No
sweeping changes!

Re: warning: massive change to conditional coding style in net?

[Eric Dumazet]

William Allen Simpson a écrit :
> Joe Perches wrote:
>> If it makes getting tcp cookies accepted difficult,
>> a reversion is simple.  That style isn't as important.
>>
> Then why make an *un*important (yet sweeping) change?

Because this is time :

We usually makes cleanup patches just before the release
of a new linux kernel, to minimize effects on developer trees.

We know linux-2.6.32 is about to be released by Linus,
and all major 2.6.33 patches are already queued in net-next-2.6
to be pushed to Linus as soon as the window opens.

This is the perfect time for cleanups. Doing cleanups is a good
way to learn linux code, before doing more complex things.

If you take a look at queued patches in net-next-2.6, maybe less
than 5 % are cleanups.

But this rule can be changed, if your patches are ready for inclusion,
David might revert the jumbo cleanup to ease your job. Just ask.

Re: warning: massive change to conditional coding style in net?

[Jarek Poplawski]

William Allen Simpson wrote, On 12/01/2009 05:08 PM:

> But more important, at least to my thinking, is keeping patches simple
> by conforming to the *existing* style in the section of code.  No
> sweeping changes!

How about new sections in between transit smoothly from one style
to another? (Or 50/50?)

Jarek P.

Re: warning: massive change to conditional coding style in net?

[David Miller]

From: William Allen Simpson <william.allen.simpson@gmail.com>
Date: Mon, 30 Nov 2009 05:36:21 -0500

> Over the past several days, David Miller (with help from Joe Perches)
> made sweeping changes to the format of conditional statements in the
> net tree -- the equivalent of mass patches that change spaces.
> 
> This makes writing patches for multiple versions of the tree very
> difficult, and will make future pullups problematic.  It's enough to
> make a grown man cry....  Patching conflicts everywhere!

William, you're unreasonable.

We asked you to follow a certain style, and then you immediately
complain that the style isn't followed consistently in the tree, and
therefore as a consequence you shouldn't be required to follow it.

Then Joe comes and submits patches making the tree follow the style
more consistently.  See, instead of merely complaining like you did,
he proactively did something positive.

Now you're complaining because this makes your patches harder to
maintain.

You're being difficult about this from every possible angle.  I can
only conclude that for whatever reason you don't want to have any
requirements made upon you for your submission.  You've handled this
negatively in every way possible, and every step of the way.  No
matter how hard other people have been to try and help you, you've
continued to do this.

Whereas if you had merely fixed up the coding style as I and others
have asked you, your code would be in my tree weeks ago.

Why is it such a huge deal to follow the coding style that the
maintainer (and other prominent developers of that subsystem) are
asking you to follow?

Re: warning: massive change to conditional coding style in net?

[William Allen Simpson]

David Miller wrote:
> William, you're unreasonable.
> 
I've waited a day before replying to this message, so that my anger
at your false and misleading statements could be carefully removed.

Maybe you're smiling and genial as you write your responses, but it
comes across as throwing a temper tantrum.

Meantime, I brought this topic to a broader audience to see whether
there was some general consensus that could be documented.

Your personal /ad hominem/ attacks are not appreciated.


> We asked you to follow a certain style, and then you immediately
> complain that the style isn't followed consistently in the tree, and
> therefore as a consequence you shouldn't be required to follow it.
> 
Whenever you've asked for an arbitrary and capricious change, I've
pointed to existing code -- not _com_plaining, *ex*plaining.

Before beginning, I read the Documentation.  I started with an existing
patch that had already been approved by you during its RFC review, and
carefully followed the existing coding practices.

In each section of code, I've followed its *existing* style.  Because
different sections of the code often have different styles, that means
the patch will be cleaner.  IMnsHO, clean patches are easier to review.

Where the code section was entirely new, I followed a style that is
well represented in the Linux tree as a whole (and the TCP code in
particular), although it is not a majority style.


> Then Joe comes and submits patches making the tree follow the style
> more consistently.  See, instead of merely complaining like you did,
> he proactively did something positive.
> 
First of all, this is not *THE* style.  Your style is not documented in
CodingStyle.  It may be your preference, but there is no agreement, not
even in patches that you've approved and applied in recent months.

Secondly, Joe was not "proactive".  You solicited the patches:

   http://patchwork.ozlabs.org/patch/39072/

Thirdly, there is disagreement about the "positive" nature.  For example:

   "Rather than playing with the dangling operator format which seems to
   be a coding style that only David cares about...."

   http://patchwork.kernel.org/patch/63847/


> Now you're complaining because this makes your patches harder to
> maintain.
> 
You betcha!  And I'm quite sure I'm not the only contributor impacted.


> Whereas if you had merely fixed up the coding style as I and others
> have asked you, your code would be in my tree weeks ago.
> 
NOT TRUE!  This was a *recent* request by you, and a change to code
previous Ack'd weeks ago by other reviewers.

Another recent example is initializing a sysctl (present in every patch
for TWO MONTHS) that you've suddenly declared "extremely non-intuitive",
and "doesn't make any sense."  As I pointed to the origin in syncookies,
you changed syncookies....  (Andi Kleen, such a bad coder.)

An example from a few iterations ago: you required the "const" be
removed from my inline functions, notwithstanding that EVERY OTHER
FUNCTION in the linux/tcp.h header uses that form.  Of course, using
const there is standard C.

Over and over, I've followed existing coding practices.  Over and over,
you've thrown roadblocks into the process, since your comment that this
was "some odd-ball feature" back in early October....

Each time I've posted a patch series, one or two usually trivial changes
are requested.  Heck, changes to comments!

If these changes had been mentioned a month or two ago, as part of a
thorough review, it could have been discussed earlier.  Instead, it's
like being nibbled by mice.

Re: warning: massive change to conditional coding style in net?

[Eric Dumazet]

William Allen Simpson a écrit :
 
> If these changes had been mentioned a month or two ago, as part of a
> thorough review, it could have been discussed earlier.  Instead, it's
> like being nibbled by mice.

William,

This is absolutely _not_ possible to comment a 1000+ lines patches in one shot,
being your patch or not.

It would take _hours_ of work, even for David.

We use a step-by-step process, with limited feedbacks, because nobody is paid
to make your life easier. We patiently gave you a lot of advices and yet
you complain again and again.

After 15/30 minutes of reviewing patches, and collecting some suggestions,
the average reviewer stops its review, sends a feedback, and waits for next
patch submission. [hoping patch author will be pro_active and check all its
patches, not the precise points that were specificaly raised.]

Yes, sometime we notice a point at round eleven, instead of first/second round,
because more important things were noticed at prior rounds.

I honestly hope you change your mind, or I wont even read your next patch
submissions, wait for the official RFC and code the damn thing myself or wait for
some kind developper willing to do the job, in the normal process.

You can copy this mail to all man kind, it wont make your patches magically
ready for inclusion.

Respectfully,
Eric

Re: warning: massive change to conditional coding style in net?

[David Miller]

From: William Allen Simpson <william.allen.simpson@gmail.com>
Date: Tue, 01 Dec 2009 12:56:40 -0500

> Heck, changes to comments!

Comments help people understand your code.  Long after you're gone and
no longer around, other people will need to read your code, maintain
it, and fix bugs in it.

That's why well written and properly formatted comments are important.

Again, nobody is picking on your specifically, everyone goes through
this review process.  What is unique about you is that you're the only
one who throws rotten tomatoes back at the reviewers instead of simply
fixing up your patch set and resubmitting.

If the changes requested are so simple and trivial, then it should
take you but a few minutes to respin your patches with the fixes
included.

It definitely takes you more effort to write all of these angry
emails, but how you spend your time is your choice.