| Message ID | 4B13A025.7000103@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;