Re: Merging of completely unreviewed drivers
Jörn Engel <[EMAIL PROTECTED]> writes:
> I strongly disagree. Machine-generated warnings are a great way of
> quickly locating a large amount of questionable code in an otherwise
> overwhelming haystack. It doesn't even matter much, which warnings you
> look for. Almost all code checkers find the same hotspots.
I think you misunderstood. Of course I'm not against warnings in
general. I'm rather talking about _authority_ of human vs machine,
in this specific ("measuring" code complexity) case.
--
Krzysztof Halasa
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Jörn Engel [EMAIL PROTECTED] writes: I strongly disagree. Machine-generated warnings are a great way of quickly locating a large amount of questionable code in an otherwise overwhelming haystack. It doesn't even matter much, which warnings you look for. Almost all code checkers find the same hotspots. I think you misunderstood. Of course I'm not against warnings in general. I'm rather talking about _authority_ of human vs machine, in this specific (measuring code complexity) case. -- Krzysztof Halasa -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri, 22 February 2008 23:28:58 +0100, Krzysztof Halasa wrote: > Al Viro <[EMAIL PROTECTED]> writes: > > > IMO the line length overruns make good warnings. Not as in "here's a cheap > > way to get more changesets", but as in "that code might have other problems > > nearby" kind of heuristics. > > Sure, it does. However the human looking at the code is far better at > spotting such problems. Machine-generated warnings are great when the > machine is actually better than human. I strongly disagree. Machine-generated warnings are a great way of quickly locating a large amount of questionable code in an otherwise overwhelming haystack. It doesn't even matter much, which warnings you look for. Almost all code checkers find the same hotspots. But there is a catch. If you have an over-eager warning police that "fixes all the warnings", the warnings may be gone, but the very real problems in near vicinity are not. Not to mention new problems introduced by those claimed "fixes". One fun hobby in my last job was to write a new code checker and locate those problem areas hidden behind warning-free code. I had to write a new checker so I would see below the polish of "fixes". The actual problems found by the checker were often trivial and near-meaningless. But those warnings are not the point at all, quite the contrary. The only important thing was "that code might have other problems nearby". Note one scary consequence: code checkers in the wrong hands are actively harmful. Jörn -- One of my most productive days was throwing away 1000 lines of code. -- Ken Thompson. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Sun, 24 Feb 2008, David Newall wrote: > > > which talks more about what matters - too deep indentation. > > What's too deep? Is the following too deep? It would be, if it weren't artificially so, for violates several kernel coding standards, one being that the "case" labels indent with the switch, not under it (the other being the placement of braces). > (Yes, I know, "we don't indent 'case' because it consumes too much > room." No, that's not it at all. We don't indent 'case' because it matches with the 'switch', not because of any room issues. > That's inconsistent with the rest of normal indenting style, and > a poor excuse to keep within an obsolete and unnecessary restriction.) It's not at all inconsistent. It's just making clear how the parts of the function group together. Indenting a case-statement an extra level is as stupid as indenting "else" one extra level from the "if ()" it goes together with. Do you think that would be sane? The fact that the 'case' thing is technically parsed as a separate statement in C doesn't change anything. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Linus Torvalds wrote:
> On Sat, 23 Feb 2008, David Newall wrote:
>
>> Do you actually get 80 columns wide on it?
>>
>
> Do people really care that deeply?
> ...
> And do I find lines longer than 80 charactes unreadable? Hell no.
>
I care, yes. I've found my code looks much prettier, with attendant
improvement in ease of understanding, since I stopped being so anal
about 80 columns. The width of the code, that is first to last
non-blank on each line, is about the same, not because I work to keep
it narrow, but because most statements just are narrow. Sometimes I do
get really wide statements, for example when using deep data structures,
especially as parameters in procedure calls, and this is easier to read
than having to break the line.
I honestly think the reason we used to insist on lines less than 80
characters was because on an 80 character screen, you get slightly
better readability by choosing where to break each line than simply
letting the hardware do it. We don't have the physical limit any more,
so we don't need to impose it structurally.
It's about readability, and with due respect, people who've never tried
it aren't qualified to comment.
> which talks more about what matters - too deep indentation.
What's too deep? Is the following too deep? It's common enough, other
than my refusal to relax consistent indenting style for switch bodies.
The code is readable, and breaking it into multiple procedures just to
de-indent is often impossible, and rarely readable. With a strict 80
character limit, the meat in the sandwich is left with only 20 or so
characters in which to fit. Add a nested switch, and there's virtually
no space left for code.
123456789012345678901234567890123456789012345678901234567890123456 (70)
int procedure(param list)
{
switch (condition)
{
case value:
if (another_condition)
{
if (variant)
meat_in_sandwich;
} else {
code;
}
case value2:
switch (sub_condition)
{
case sub_value:
if (final_test)
{
something(
NULL,
1,
"two");
}
}
}
}
(Yes, I know, "we don't indent 'case' because it consumes too much
room." That's inconsistent with the rest of normal indenting style, and
a poor excuse to keep within an obsolete and unnecessary restriction.)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Jan Engelhardt wrote:
> static void blah(void)
> {
> if (foo) {
> bar;
> bar2;
> return;
> }
> if (this) {
> that;
> that2;
> return;
> }
> /* yay, got rid of two levels of indent! */
> good day;
> good day2;
> }
I like this style. It's more readable than the alternative that you
showed. If you hate returns mid-procedure, as some purists do, the
following is also good:
static void blah(void)
{
if (foo) {
bar;
bar2;
} else if (this) {
that;
that2;
} else {
good day;
good day2;
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Pavel Machek wrote: > On Sat 2008-02-23 23:08:58, David Newall wrote: > >> Pavel Machek wrote: >> >>> On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote: >>> >>> Pavel Machek <[EMAIL PROTECTED]> writes: > Zaurus is one example, second is small screen where you need big font > to keep it readable (x60 on desk). > > Come on, are you doing Linux kernel development on PDA? > > Actually, I'd like to. There's a lot to fix on zaurus. Bit corruption > while sleeping is high on list, but I guess I should move out of > 2.6.16, first. > > >>> I review patches on it, sometimes, yes. >>> >>> >> Do you actually get 80 columns wide on it? >> > > No, something like 62... but it is usually enough. x60 is about 100 > columns wide (big font needed). Then it's a silly example to raise in a serious discussion of this type. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Sat, 23 Feb 2008, David Newall wrote: > > Do you actually get 80 columns wide on it? Do people really care that deeply? I still sometimes use small terminal windows - for a while I had my default terminal come up as 100x40, but I'm back to the standard 80x24, and while I often resize them, I certainly don't always. And do I find lines longer than 80 charactes unreadable? Hell no. Quite frankly, on a 80x24 display, I'll take long lines over split-up ones *any* day. For things like doing "git grep xyzzy", I'd *much* rather get the occasional long line that wraps (or, if I'm in "less -S", that I have to press right-arrow to see), than see just a meaningless fragment because somebody decided that they should always fit in 80 characters. So *consistently* long lines are the problem, not the occasional one. The occasional one is likely more readable as it is, than split up. Here's an example from code that actually looks pretty good in general: static unsigned long calc_delta_mine(unsigned long delta_exec, unsigned long weight, struct load_weight *lw) and look around that function in general: it's doesn't match the coding standard, but also compare the output of git grep calc_delta_mine with the output of something like git grep update_load_sub which actually shows you what the calling convention is. So putting that long function definition on one line would make it a 108-character line or somethign like that, but it would have advantages too. It would have advantages for anything that is line-based (I use grep for *everything*, but maybe I'm just odd), but it would also actually be more readable for the people who have bigger windows. But my point is, some of those advantages remain even with small terminals, and quite often the downsides aren't even all that huge. Most editors wrap or chop the line according to your preferences (mine are personally to chop), and if it's a fairly uncommon thing, those downsides shrink further. Is 108 characters perhaps *too* long? In the above case it probably is, since the downside of splitting the patch is pretty small (it's a static function, only used in that file, the "grep" argument is weak, yadda yadda). But I'm just saying that it's not 100% obvious *even*if* you're on a 80x24 terminal, and in some other cases the downside of splitting the line can be much bigger (strings or more spread-out function calls and declarations etc). The line length problem would probably be better attacked as something more akin to the rule - do a rolling window of last non-empty lines (n ~ 15 or so) - if more than of those lines were longer than 72 charactes, somethign is wrong (m ~ 5 or so). which talks more about what matters - too deep indentation. And also attacks the problem that is really relevant: it's that kind of code that ends up being unreadable because so *much* of it is cut off or wrapped. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Feb 21 2008 22:37, Ray Lee wrote:
>On Thu, Feb 21, 2008 at 7:13 PM, Linus Torvalds
><[EMAIL PROTECTED]> wrote:
>> So I'd be happier with warnings about deep indentation (but how do you
>> count it? Will people then try to fake things out by using 4-space indents
>> and then "deep" indentations will look like just a couple of tabs?)
>
>I suspect that 90% of the cases that people really care about would
>get caught successfully just by counting brace depth.
>
>ie, by looking at { { {} {} {{{}{}}} } } I bet you can tell me which
>section should have been pulled out into a separate routine.
Not only that. By clever branch factoring, you can possibly get yourself
rid of lots of deep levels. As in:
static void blah(void)
{
if (foo) {
bar;
bar2;
} else {
if (this) {
that;
that2;
} else {
bad day;
bad day2;
}
}
}
xfrmd:
static void blah(void)
{
if (foo) {
bar;
bar2;
return;
}
if (this) {
that;
that2;
return;
}
/* yay, got rid of two levels of indent! */
good day;
good day2;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Sat 2008-02-23 23:08:58, David Newall wrote: > Pavel Machek wrote: > > On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote: > > > >> Pavel Machek <[EMAIL PROTECTED]> writes: > >> > >> > >>> Zaurus is one example, second is small screen where you need big font > >>> to keep it readable (x60 on desk). > >>> > >> Come on, are you doing Linux kernel development on PDA? Actually, I'd like to. There's a lot to fix on zaurus. Bit corruption while sleeping is high on list, but I guess I should move out of 2.6.16, first. > > I review patches on it, sometimes, yes. > > > > Do you actually get 80 columns wide on it? No, something like 62... but it is usually enough. x60 is about 100 columns wide (big font needed). Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Pavel Machek <[EMAIL PROTECTED]> writes: >> Come on, are you doing Linux kernel development on PDA? > > I review patches on it, sometimes, yes. I take it the "sometimes" is the key word :-) -- Krzysztof Halasa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Pavel Machek wrote: > On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote: > >> Pavel Machek <[EMAIL PROTECTED]> writes: >> >> >>> Zaurus is one example, second is small screen where you need big font >>> to keep it readable (x60 on desk). >>> >> Come on, are you doing Linux kernel development on PDA? >> > > I review patches on it, sometimes, yes. > Do you actually get 80 columns wide on it? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote: > Pavel Machek <[EMAIL PROTECTED]> writes: > > > Zaurus is one example, second is small screen where you need big font > > to keep it readable (x60 on desk). > > Come on, are you doing Linux kernel development on PDA? I review patches on it, sometimes, yes. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote: Pavel Machek [EMAIL PROTECTED] writes: Zaurus is one example, second is small screen where you need big font to keep it readable (x60 on desk). Come on, are you doing Linux kernel development on PDA? I review patches on it, sometimes, yes. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Pavel Machek wrote: On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote: Pavel Machek [EMAIL PROTECTED] writes: Zaurus is one example, second is small screen where you need big font to keep it readable (x60 on desk). Come on, are you doing Linux kernel development on PDA? I review patches on it, sometimes, yes. Do you actually get 80 columns wide on it? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Pavel Machek [EMAIL PROTECTED] writes: Come on, are you doing Linux kernel development on PDA? I review patches on it, sometimes, yes. I take it the sometimes is the key word :-) -- Krzysztof Halasa -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Sat 2008-02-23 23:08:58, David Newall wrote: Pavel Machek wrote: On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote: Pavel Machek [EMAIL PROTECTED] writes: Zaurus is one example, second is small screen where you need big font to keep it readable (x60 on desk). Come on, are you doing Linux kernel development on PDA? Actually, I'd like to. There's a lot to fix on zaurus. Bit corruption while sleeping is high on list, but I guess I should move out of 2.6.16, first. I review patches on it, sometimes, yes. Do you actually get 80 columns wide on it? No, something like 62... but it is usually enough. x60 is about 100 columns wide (big font needed). Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Feb 21 2008 22:37, Ray Lee wrote:
On Thu, Feb 21, 2008 at 7:13 PM, Linus Torvalds
[EMAIL PROTECTED] wrote:
So I'd be happier with warnings about deep indentation (but how do you
count it? Will people then try to fake things out by using 4-space indents
and then deep indentations will look like just a couple of tabs?)
I suspect that 90% of the cases that people really care about would
get caught successfully just by counting brace depth.
ie, by looking at { { {} {} {{{}{}}} } } I bet you can tell me which
section should have been pulled out into a separate routine.
Not only that. By clever branch factoring, you can possibly get yourself
rid of lots of deep levels. As in:
static void blah(void)
{
if (foo) {
bar;
bar2;
} else {
if (this) {
that;
that2;
} else {
bad day;
bad day2;
}
}
}
xfrmd:
static void blah(void)
{
if (foo) {
bar;
bar2;
return;
}
if (this) {
that;
that2;
return;
}
/* yay, got rid of two levels of indent! */
good day;
good day2;
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Sat, 23 Feb 2008, David Newall wrote: Do you actually get 80 columns wide on it? Do people really care that deeply? I still sometimes use small terminal windows - for a while I had my default terminal come up as 100x40, but I'm back to the standard 80x24, and while I often resize them, I certainly don't always. And do I find lines longer than 80 charactes unreadable? Hell no. Quite frankly, on a 80x24 display, I'll take long lines over split-up ones *any* day. For things like doing git grep xyzzy, I'd *much* rather get the occasional long line that wraps (or, if I'm in less -S, that I have to press right-arrow to see), than see just a meaningless fragment because somebody decided that they should always fit in 80 characters. So *consistently* long lines are the problem, not the occasional one. The occasional one is likely more readable as it is, than split up. Here's an example from code that actually looks pretty good in general: static unsigned long calc_delta_mine(unsigned long delta_exec, unsigned long weight, struct load_weight *lw) and look around that function in general: it's doesn't match the coding standard, but also compare the output of git grep calc_delta_mine with the output of something like git grep update_load_sub which actually shows you what the calling convention is. So putting that long function definition on one line would make it a 108-character line or somethign like that, but it would have advantages too. It would have advantages for anything that is line-based (I use grep for *everything*, but maybe I'm just odd), but it would also actually be more readable for the people who have bigger windows. But my point is, some of those advantages remain even with small terminals, and quite often the downsides aren't even all that huge. Most editors wrap or chop the line according to your preferences (mine are personally to chop), and if it's a fairly uncommon thing, those downsides shrink further. Is 108 characters perhaps *too* long? In the above case it probably is, since the downside of splitting the patch is pretty small (it's a static function, only used in that file, the grep argument is weak, yadda yadda). But I'm just saying that it's not 100% obvious *even*if* you're on a 80x24 terminal, and in some other cases the downside of splitting the line can be much bigger (strings or more spread-out function calls and declarations etc). The line length problem would probably be better attacked as something more akin to the rule - do a rolling window of n last non-empty lines (n ~ 15 or so) - if more than m of those lines were longer than 72 charactes, somethign is wrong (m ~ 5 or so). which talks more about what matters - too deep indentation. And also attacks the problem that is really relevant: it's that kind of code that ends up being unreadable because so *much* of it is cut off or wrapped. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Pavel Machek wrote: On Sat 2008-02-23 23:08:58, David Newall wrote: Pavel Machek wrote: On Fri 2008-02-22 23:44:09, Krzysztof Halasa wrote: Pavel Machek [EMAIL PROTECTED] writes: Zaurus is one example, second is small screen where you need big font to keep it readable (x60 on desk). Come on, are you doing Linux kernel development on PDA? Actually, I'd like to. There's a lot to fix on zaurus. Bit corruption while sleeping is high on list, but I guess I should move out of 2.6.16, first. I review patches on it, sometimes, yes. Do you actually get 80 columns wide on it? No, something like 62... but it is usually enough. x60 is about 100 columns wide (big font needed). Then it's a silly example to raise in a serious discussion of this type. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Jan Engelhardt wrote:
static void blah(void)
{
if (foo) {
bar;
bar2;
return;
}
if (this) {
that;
that2;
return;
}
/* yay, got rid of two levels of indent! */
good day;
good day2;
}
I like this style. It's more readable than the alternative that you
showed. If you hate returns mid-procedure, as some purists do, the
following is also good:
static void blah(void)
{
if (foo) {
bar;
bar2;
} else if (this) {
that;
that2;
} else {
good day;
good day2;
}
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Linus Torvalds wrote:
On Sat, 23 Feb 2008, David Newall wrote:
Do you actually get 80 columns wide on it?
Do people really care that deeply?
...
And do I find lines longer than 80 charactes unreadable? Hell no.
I care, yes. I've found my code looks much prettier, with attendant
improvement in ease of understanding, since I stopped being so anal
about 80 columns. The width of the code, that is first to last
non-blank on each line, is about the same, not because I work to keep
it narrow, but because most statements just are narrow. Sometimes I do
get really wide statements, for example when using deep data structures,
especially as parameters in procedure calls, and this is easier to read
than having to break the line.
I honestly think the reason we used to insist on lines less than 80
characters was because on an 80 character screen, you get slightly
better readability by choosing where to break each line than simply
letting the hardware do it. We don't have the physical limit any more,
so we don't need to impose it structurally.
It's about readability, and with due respect, people who've never tried
it aren't qualified to comment.
which talks more about what matters - too deep indentation.
What's too deep? Is the following too deep? It's common enough, other
than my refusal to relax consistent indenting style for switch bodies.
The code is readable, and breaking it into multiple procedures just to
de-indent is often impossible, and rarely readable. With a strict 80
character limit, the meat in the sandwich is left with only 20 or so
characters in which to fit. Add a nested switch, and there's virtually
no space left for code.
123456789012345678901234567890123456789012345678901234567890123456 (70)
int procedure(param list)
{
switch (condition)
{
case value:
if (another_condition)
{
if (variant)
meat_in_sandwich;
} else {
code;
}
case value2:
switch (sub_condition)
{
case sub_value:
if (final_test)
{
something(
NULL,
1,
two);
}
}
}
}
(Yes, I know, we don't indent 'case' because it consumes too much
room. That's inconsistent with the rest of normal indenting style, and
a poor excuse to keep within an obsolete and unnecessary restriction.)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Sun, 24 Feb 2008, David Newall wrote: which talks more about what matters - too deep indentation. What's too deep? Is the following too deep? It would be, if it weren't artificially so, for violates several kernel coding standards, one being that the case labels indent with the switch, not under it (the other being the placement of braces). (Yes, I know, we don't indent 'case' because it consumes too much room. No, that's not it at all. We don't indent 'case' because it matches with the 'switch', not because of any room issues. That's inconsistent with the rest of normal indenting style, and a poor excuse to keep within an obsolete and unnecessary restriction.) It's not at all inconsistent. It's just making clear how the parts of the function group together. Indenting a case-statement an extra level is as stupid as indenting else one extra level from the if () it goes together with. Do you think that would be sane? The fact that the 'case' thing is technically parsed as a separate statement in C doesn't change anything. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri, 22 February 2008 23:28:58 +0100, Krzysztof Halasa wrote: Al Viro [EMAIL PROTECTED] writes: IMO the line length overruns make good warnings. Not as in here's a cheap way to get more changesets, but as in that code might have other problems nearby kind of heuristics. Sure, it does. However the human looking at the code is far better at spotting such problems. Machine-generated warnings are great when the machine is actually better than human. I strongly disagree. Machine-generated warnings are a great way of quickly locating a large amount of questionable code in an otherwise overwhelming haystack. It doesn't even matter much, which warnings you look for. Almost all code checkers find the same hotspots. But there is a catch. If you have an over-eager warning police that fixes all the warnings, the warnings may be gone, but the very real problems in near vicinity are not. Not to mention new problems introduced by those claimed fixes. One fun hobby in my last job was to write a new code checker and locate those problem areas hidden behind warning-free code. I had to write a new checker so I would see below the polish of fixes. The actual problems found by the checker were often trivial and near-meaningless. But those warnings are not the point at all, quite the contrary. The only important thing was that code might have other problems nearby. Note one scary consequence: code checkers in the wrong hands are actively harmful. Jörn -- One of my most productive days was throwing away 1000 lines of code. -- Ken Thompson. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 11:59:35PM +0100, Krzysztof Halasa wrote: > Sure - because email is not C code. > > Actually you don't "read" C code, word by word, as you read books - do > you? If it's decently written - sure, why not? Unfortunately, more common case is somewhere between the writing on the lavatory wall and appartment lease agreement, with several high school essays mixed in... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
Peter Zijlstra <[EMAIL PROTECTED]> writes: > So, yes, I have the screen estate for very long lines, but I find that > long lines require more effort to read (that very much includes leading > whitespace). Also, since long lines are rare (and they should be, if you > nest too deep you have other issues) accommodating them would waste a > lot of screen estate otherwise useful for another column of text. Either they are rare and you can wrap them and still use 80 columns, or it turns out they are not so rare and you may want to use wider windows (not necessarily 132 but perhaps 100). I think the question isn't if they are rare or not, or if people have 3 * 1920 pixels/line or just 1280. The question is: is the code more readable with hard limit equal to 80 characters, or maybe is it better to limit code block complexity instead, and let the maximum number of those small pictures in a line alone? (Limiting at 132 would have technical sense IMHO). Better code readability = less bugs without any additional effort. > Even with e-mail, I can easily show over 200 characters wide with a > large font (say 11pt) but find it harder to read emails that don't > nicely wrap at 78. Sure - because email is not C code. Actually you don't "read" C code, word by word, as you read books - do you? -- Krzysztof Halasa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Pavel Machek <[EMAIL PROTECTED]> writes: > Zaurus is one example, second is small screen where you need big font > to keep it readable (x60 on desk). Come on, are you doing Linux kernel development on PDA? -- Krzysztof Halasa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Linus Torvalds <[EMAIL PROTECTED]> writes: > Will people then try to fake things out by using 4-space indents > and then "deep" indentations will look like just a couple of tabs?) There is no point in faking it as it's only advisory, it's to help the author who should be free to ignore the advice. People upstream won't be fooled by some cheap tab tricks I guess. -- Krzysztof Halasa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Al Viro <[EMAIL PROTECTED]> writes: > IMO the line length overruns make good warnings. Not as in "here's a cheap > way to get more changesets", but as in "that code might have other problems > nearby" kind of heuristics. Sure, it does. However the human looking at the code is far better at spotting such problems. Machine-generated warnings are great when the machine is actually better than human. Anyway, warnings are one thing and line limit is another. We may raise the limit leaving the 80-chars warning in place. Unless there are too many false positives, of course. -- Krzysztof Halasa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 02:20:12PM -0500, Jeff Garzik wrote: > Ingo Molnar wrote: >> 2) you might know that Deja-Vu moment when you look at a new patch that >> has been submitted to lkml and you have a strange, weird "feeling" >> that there's something wrong about the patch. >> It's totally subconscious, and you take a closer look and a few >> seconds later you find a real bug in the code. >> That "feeling" i believe comes from a fundamental property of how >> human vision is connected to the human brain: pattern matching. Really >> good programmers have built a "library" of patterns of "good" and >> "bad" looking coding practices. >> If a patch or if a file has a clean _style_, bugs and deeper >> structural problems often stand out like a sore thumb. But if the > [...] > >> The best programmers are the ones who have a good eye for details - >> and that subconsciously extends to "style details" too. I've yet to >> see a _single_ example of a good, experienced kernel programmer who >> writes code that looks absolutely careless and sloppy, but which is >> top-notch otherwise. (Newbies will make style mistakes a lot more >> often - and for them checkpatch is a nice and easy experience at >> reading other people's code and trying to learn the style of the >> kernel.) > [...] > >> 4) there's a psychological effect as well: clean _looking_ code is >> more attractive to coders to improve upon. Once the code _looks_ clean >> (mechanically), the people with the real structural cleanups are not >> far away either. Code that just looks nice is simply more of a >> pleasure to work with and to improve, so there's a strong >> psychological relationship between the "small, seemingly unimportant >> details" cleanups and the real, structural cleanups. > > The above deserved to be quoted... just because I agree with all of it so > strongly :) > > Bugs really do "hide" in ugly code, in part because my brain has been > optimized to review clean code. > > Like everything else in life, one must strike a balance between picking > style nits with someone's patch, and making honest criticisms of a patch > because said patch is too "unclean" to be reviewed by anyone. I totally agree with all of this. checkpatch.pl is a useful tool to use, and is quite handy for helping the kernel code for all of the above reasons. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri 2008-02-22 01:05:26, Krzysztof Halasa wrote: > Jeff Garzik <[EMAIL PROTECTED]> writes: > > > If a driver is full of lines of length >80, that's a problem. > > I'm not sure. > We all have more than 80-chars wide displays for years, don't we? The No. Zaurus is one example, second is small screen where you need big font to keep it readable (x60 on desk). Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Thu 2008-02-21 14:08:55, Arjan van de Ven wrote: > On Thu, 21 Feb 2008 23:01:24 +0200 > Adrian Bunk <[EMAIL PROTECTED]> wrote: > > > [ Linus Added to the To: since I want to hear his opinion on this > > issue. ] > > > > On Thu, Feb 21, 2008 at 12:28:55PM -0800, Roland Dreier wrote: > > > > This driver should really have gotten some review before being > > > > included in the kernel. > > > > > > > Even a simple checkpatch run finds more than > 250 stylistic > > > > errors (not code bugs but cases where the driver violates the > > > > standard code formatting rules of kernel code). > > > > > > Linus has strongly stated that we should merge hardware drivers > > > early, and I agree: although the nes driver clearly needs more > > > work, there's no advantage to users with the hardware in forcing > > > them to wait for 2.6.26 to merge the driver, since they'll just > > > have to patch the grungy code in themselves anyway. And by merging > > > the driver early, we get fixed up for any tree-wide changes and > > > allow janitors to help with the cleanup. > > > > Is it really intended to merge drivers without _any_ kind of review? > > No of course not. > > I totally agree we should be more agressive in merging drivers earlier. > A minimal review needs to happen so for a few things imo > 1) That the driver doesn't break the build > 2) That the driver has no obvious huge security holes >(this is a big deal for unsuspecting users) > 3) that there's not an obscene amount of "uses deprecated api" compiler > warnings >(since those are annoying for everyone else) > 4) that people who don't have the hardware are not negatively affected >(say crashes without the hw or so) 5) does not introduce new and ugly user-kernel we'll have problems fixing/removing? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Ingo Molnar wrote: 2) you might know that Deja-Vu moment when you look at a new patch that has been submitted to lkml and you have a strange, weird "feeling" that there's something wrong about the patch. It's totally subconscious, and you take a closer look and a few seconds later you find a real bug in the code. That "feeling" i believe comes from a fundamental property of how human vision is connected to the human brain: pattern matching. Really good programmers have built a "library" of patterns of "good" and "bad" looking coding practices. If a patch or if a file has a clean _style_, bugs and deeper structural problems often stand out like a sore thumb. But if the [...] The best programmers are the ones who have a good eye for details - and that subconsciously extends to "style details" too. I've yet to see a _single_ example of a good, experienced kernel programmer who writes code that looks absolutely careless and sloppy, but which is top-notch otherwise. (Newbies will make style mistakes a lot more often - and for them checkpatch is a nice and easy experience at reading other people's code and trying to learn the style of the kernel.) [...] 4) there's a psychological effect as well: clean _looking_ code is more attractive to coders to improve upon. Once the code _looks_ clean (mechanically), the people with the real structural cleanups are not far away either. Code that just looks nice is simply more of a pleasure to work with and to improve, so there's a strong psychological relationship between the "small, seemingly unimportant details" cleanups and the real, structural cleanups. The above deserved to be quoted... just because I agree with all of it so strongly :) Bugs really do "hide" in ugly code, in part because my brain has been optimized to review clean code. Like everything else in life, one must strike a balance between picking style nits with someone's patch, and making honest criticisms of a patch because said patch is too "unclean" to be reviewed by anyone. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 7:54 PM, Ingo Molnar <[EMAIL PROTECTED]> wrote: > If a patch or if a file has a clean _style_, bugs and deeper > structural problems often stand out like a sore thumb. But if the > code is peppered with random style noise, it's a lot harder (for me > at least) to notice real bugs. I can notice bugs in a squeeky clean > code base about 5 times easier than in a noisy codebase. This effect > alone makes checkpatch indispensible for the scheduler and for > arch/x86. I also appreciate style uniformity in kernel code. My (limited) experience with checkpatch is that most checkpatch complaints are easy to resolve. Bart Van Assche. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 04:17:17PM +0100, Peter Zijlstra wrote: > Even with e-mail, I can easily show over 200 characters wide with a > large font (say 11pt) but find it harder to read emails that don't > nicely wrap at 78. So much so that I often find myself not reading the > mail, or restyling it if I find it important enough to read anyway. Yes, ditto. And since most of my patch review is done inside mutt... -- John W. Linville [EMAIL PROTECTED] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
* Linus Torvalds <[EMAIL PROTECTED]> wrote: > I'm personally of the opinion that a lot of checkpatch "fixes" are > anything but. That mainly concerns fixing overlong lines (where the > "fixed" version is usually worse than the original), but it's been > true for some other warnings too. that was certainly the case for the earlier checkpatch releases which treated overlong lines as an error. So here's a quick list of negative and positive aspects of current versions of checkpatch, as i see them. But let me first declare it that when scripts/checkpatch.pl was initially merged last year i immediately ran it over my own files and became a deep sceptic of it. (check the lkml archives, i complained alot about it) Now i've got more than half a year of experience with using checkpatch as an integral part of scheduler maintenance, and we've now got 4 months of experience with using checkpatch in arch/x86 maintenance. Based on this first hand experience, my opinion about checkpatch has changed, rather radically: i now believe that checkpatch is almost as important to the long term health of our kernel development process as BitKeeper/Git turned out to be. If i had to stop using it today, it would be almost as bad of a step backwards to me as if we had to migrate the kernel source code control to CVS. Lets see the Bad Side of checkpatch: 1) checkpatch "errors" shouldnt be taken too seriously for newly introduced "leaf" driver code, which code we dont at all know whether we'll be maintaining in any serious manner in the future. Slowing down a submission by requirig it to pass checkpatch is not as clear-cut as it is for core infrastructure and architecture code. It's far more important to get _any_ code to users (as long as it's not outright harmful) than to nitpick about style details. 2) it still has some false positives. (They are quite rare in the latest versions, about 1 out of 100 for code that is already "clean". I send them over to Andy whenever i see them, and they get fixed quickly. The false positives were a big annoyance in early checkpatch.pl versions, these days they are not - to me at least.) 3) it's _really_ annoying when sometimes i stumble over some old, crufty piece of code that according to checkpatch is in high need of some good, thorough cleanup - and when i take a look at the code it turns out that the original author of that crap piece of code turns out to be ... me. Those moments can be pretty embarrasing and sobering ;-) The Good Side of checkpatch (and here i'll only list the non-obvious advantages): 1) 90% of the scheduler related checkpatch fixes today you'll never recognize in a commit! The fixes all happen before code is submitted, and the fixes are seemlessly embedded in nice looking patches. (in that sense checkpatch is a bit like lockdep: 90% of the errors they detect wont hit lkml, ever.) 2) you might know that Deja-Vu moment when you look at a new patch that has been submitted to lkml and you have a strange, weird "feeling" that there's something wrong about the patch. It's totally subconscious, and you take a closer look and a few seconds later you find a real bug in the code. That "feeling" i believe comes from a fundamental property of how human vision is connected to the human brain: pattern matching. Really good programmers have built a "library" of patterns of "good" and "bad" looking coding practices. If a patch or if a file has a clean _style_, bugs and deeper structural problems often stand out like a sore thumb. But if the code is peppered with random style noise, it's a lot harder (for me at least) to notice real bugs. I can notice bugs in a squeeky clean code base about 5 times easier than in a noisy codebase. This effect alone makes checkpatch indispensible for the scheduler and for arch/x86. Sidenote: i dont really need fancy metrics trying to tell me how good an algorithm _truly_ is (although it certainly would be interesting to have). I can _see_ that at a glance - provided the code follows common kernel practices and a common, consistent style. Checkpatch makes visual code patterns universal and eases the human maintainance work enormously, for a 150+ KLOC subsystem like arch/x86. I'm not distracted (visually and mentally) by the thick fog of small silly details and quirks in coding style. Others might have radar eyes and radar brains, i dont :-) 3) checkpatch also keeps _my_ bugs out of the kernel in an interesting way. I'm sure many of you are like me: i've got "weaker" moments when i write rather crappy code, and i've got "stronger" moments when i'm in the flow and can write a few thousand lines of code with nary a hickup. What makes things worse is it's really hard to tell the two apart. It turns
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Sat, Feb 23, 2008 at 12:55:03AM +1030, David Newall wrote: > Bart Van Assche wrote: > > There is a reason to limit line length: scientific research has shown > > that readability of regular texts is optimal for a line length between > > 55 and 65 characters. > > Putting aside the point that we're talking code, not regular text, I've > heard that said before and I don't think it's quite like that. Perhaps > the numbers you said might assume various things such as the width of > the eye's field of view, the distance to the image and the size of each > character? I'm sure all those assumptions are baked-in to the estimate. Yet the fact remains that people's eyes are only so good and most people will be reading at similar distances from the screen. So I don't see any reason to invalidate those assumptions. FWIW, I find reading longer lines to be painful -- it is easier to loose one's place in the text. I would also echo a point Jeff Garzik made elsewhere that it is often beneficial to have multiple windows oppen side-by-side. Longer lines makes it harder to do that in a useful way. Instead the lines either wrap or just trail off the screen. See the output of sdiff for how this limits usefulness. > > My experience is that the readability of source > > code decreases when the lines are very long (more than 160 > > characters). > > The point is that the width, excluding leading and trailing white space, > is what really matters. Even deeply indented code can be a snap to > understand if you don't have to fight artificial line breaks. And we've > got a much wider -- and taller! -- space available than we had in the > old 80x24 (and 80x1) days. I'm not sure deeply indented code is ever a snap to understand. And FWIW, I'd rather deal with "artificial" line breaks than parameter lists that just stream off the side of the page. The line breaks make long parameters lists easier to digest. I'll sacrifice the occasional odd breakage of a long string. John -- John W. Linville [EMAIL PROTECTED] -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Sat, 2008-02-23 at 00:55 +1030, David Newall wrote: > Bart Van Assche wrote: > > There is a reason to limit line length: scientific research has shown > > that readability of regular texts is optimal for a line length between > > 55 and 65 characters. > > Putting aside the point that we're talking code, not regular text, I've > heard that said before and I don't think it's quite like that. Perhaps > the numbers you said might assume various things such as the width of > the eye's field of view, the distance to the image and the size of each > character? Not in my experience. > > My experience is that the readability of source > > code decreases when the lines are very long (more than 160 > > characters). > > The point is that the width, excluding leading and trailing white space, > is what really matters. Even deeply indented code can be a snap to > understand if you don't have to fight artificial line breaks. And we've > got a much wider -- and taller! -- space available than we had in the > old 80x24 (and 80x1) days. I have 2 24" screens running at 1920x1200 with X forced to 75dpi and use a 8pt Monospace font. (Yes, I can read that from more than 3ft away) Using a fullscreen gvim (without the icons, but with the menu) with 3 vertical splits gives me 4 columns of 113 rows and 95 chars. So, yes, I have the screen estate for very long lines, but I find that long lines require more effort to read (that very much includes leading whitespace). Also, since long lines are rare (and they should be, if you nest too deep you have other issues) accommodating them would waste a lot of screen estate otherwise useful for another column of text. Even with e-mail, I can easily show over 200 characters wide with a large font (say 11pt) but find it harder to read emails that don't nicely wrap at 78. So much so that I often find myself not reading the mail, or restyling it if I find it important enough to read anyway. Please, lets keep the 80 as a guideline, and not trip over the occasional lines that exceed it in good style (read: wrapping them would indeed give uglier code) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
Bart Van Assche wrote: > There is a reason to limit line length: scientific research has shown > that readability of regular texts is optimal for a line length between > 55 and 65 characters. Putting aside the point that we're talking code, not regular text, I've heard that said before and I don't think it's quite like that. Perhaps the numbers you said might assume various things such as the width of the eye's field of view, the distance to the image and the size of each character? > My experience is that the readability of source > code decreases when the lines are very long (more than 160 > characters). The point is that the width, excluding leading and trailing white space, is what really matters. Even deeply indented code can be a snap to understand if you don't have to fight artificial line breaks. And we've got a much wider -- and taller! -- space available than we had in the old 80x24 (and 80x1) days. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 2:46 AM, David Newall <[EMAIL PROTECTED]> wrote: > Krzysztof Halasa wrote: > > Perhaps we should increase line length limit, 132 should be fine. > > Especially useful with long printk() lines and long arithmetic > > expressions. > > Yes; or even longer. 80 characters might have made sense on a screen > when the alternative was 80 characters on a punched card, but on a > modern computer it's very restrictive. That's especially true with the > deep indents that you quickly get in C. Even short lines often need to > be split when you put a few tabs in front of them, and that makes > comprehension that bit harder, not to mention looks ugly. There is a reason to limit line length: scientific research has shown that readability of regular texts is optimal for a line length between 55 and 65 characters. My experience is that the readability of source code decreases when the lines are very long (more than 160 characters). Bart Van Assche. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri, 22 Feb 2008 01:05:26 +0100 Krzysztof Halasa <[EMAIL PROTECTED]> wrote: > Jeff Garzik <[EMAIL PROTECTED]> writes: > > > If a driver is full of lines of length >80, that's a problem. > > I'm not sure. > We all have more than 80-chars wide displays for years, don't we? The Even a vt132 serial terminal or later can do 132. Decades not years. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Thu, Feb 21, 2008 at 10:29:09PM -0800, Junio C Hamano wrote: > Linus Torvalds <[EMAIL PROTECTED]> writes: > > > So I'd be happier with warnings about deep indentation (but how do you > > count it? Will people then try to fake things out by using 4-space indents > > and then "deep" indentations will look like just a couple of tabs?) and > > against complex expressions (ie "if ((a = xyz()) == NULL) .." should just > > be split up into "a = xyz(); if (!a) ..", but there are sometimes reasons > > for those things too! > > Deep indentation should be fairly easy, given that you > already have rules in place that says "Tabs are 8 characters". > So if you find a line that begins with more than say 4 SP, you > can flag that as already bogus (i.e. "does not indent with HT"), > more than 8 SP definitely so. >... Checkpatch already has an error "use tabs not spaces". And people should realize that checkpatch is not a tool for janitors but for authors and maintainers to easily spot some of the possible problems in a driver and thereby automate some part of patch review. E.g. in this driver we are talking about checkpatch warns about the > 2000 lines over 80 characters. And that's not a surprise and a symptom when code is 6 tabs indented. If someone said fixing that should not delay the merge of a 16.500 lines driver I would agree with that since fixing that would require a huge amount of work for a not that big gain. But that a merged driver contains > 250 checkpatch errors is really not nice. Most of them are easy to fix stylistic errors that simply make the driver easier to read and whose fixing would only take a few hours altogether. [1] And the 13 checkpatch errors about volatile usage are not stuff for janitors (unless you count our number one cleanup person Al as janitor) but indicate really fishy code. cu Adrian [1] one might argue whether "easier to read" really applies when checkpatch gives errors for e.g. the usage of C99 comments, but different from overly long lines that's at least stuff that can be fixed very quickly and in a quite automatic way -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Thu, Feb 21, 2008 at 10:29:09PM -0800, Junio C Hamano wrote: Linus Torvalds [EMAIL PROTECTED] writes: So I'd be happier with warnings about deep indentation (but how do you count it? Will people then try to fake things out by using 4-space indents and then deep indentations will look like just a couple of tabs?) and against complex expressions (ie if ((a = xyz()) == NULL) .. should just be split up into a = xyz(); if (!a) .., but there are sometimes reasons for those things too! Deep indentation should be fairly easy, given that you already have rules in place that says Tabs are 8 characters. So if you find a line that begins with more than say 4 SP, you can flag that as already bogus (i.e. does not indent with HT), more than 8 SP definitely so. ... Checkpatch already has an error use tabs not spaces. And people should realize that checkpatch is not a tool for janitors but for authors and maintainers to easily spot some of the possible problems in a driver and thereby automate some part of patch review. E.g. in this driver we are talking about checkpatch warns about the 2000 lines over 80 characters. And that's not a surprise and a symptom when code is 6 tabs indented. If someone said fixing that should not delay the merge of a 16.500 lines driver I would agree with that since fixing that would require a huge amount of work for a not that big gain. But that a merged driver contains 250 checkpatch errors is really not nice. Most of them are easy to fix stylistic errors that simply make the driver easier to read and whose fixing would only take a few hours altogether. [1] And the 13 checkpatch errors about volatile usage are not stuff for janitors (unless you count our number one cleanup person Al as janitor) but indicate really fishy code. cu Adrian [1] one might argue whether easier to read really applies when checkpatch gives errors for e.g. the usage of C99 comments, but different from overly long lines that's at least stuff that can be fixed very quickly and in a quite automatic way -- Is there not promise of rain? Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. Only a promise, Lao Er said. Pearl S. Buck - Dragon Seed -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri, 22 Feb 2008 01:05:26 +0100 Krzysztof Halasa [EMAIL PROTECTED] wrote: Jeff Garzik [EMAIL PROTECTED] writes: If a driver is full of lines of length 80, that's a problem. I'm not sure. We all have more than 80-chars wide displays for years, don't we? The Even a vt132 serial terminal or later can do 132. Decades not years. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 2:46 AM, David Newall [EMAIL PROTECTED] wrote: Krzysztof Halasa wrote: Perhaps we should increase line length limit, 132 should be fine. Especially useful with long printk() lines and long arithmetic expressions. Yes; or even longer. 80 characters might have made sense on a screen when the alternative was 80 characters on a punched card, but on a modern computer it's very restrictive. That's especially true with the deep indents that you quickly get in C. Even short lines often need to be split when you put a few tabs in front of them, and that makes comprehension that bit harder, not to mention looks ugly. There is a reason to limit line length: scientific research has shown that readability of regular texts is optimal for a line length between 55 and 65 characters. My experience is that the readability of source code decreases when the lines are very long (more than 160 characters). Bart Van Assche. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
Bart Van Assche wrote: There is a reason to limit line length: scientific research has shown that readability of regular texts is optimal for a line length between 55 and 65 characters. Putting aside the point that we're talking code, not regular text, I've heard that said before and I don't think it's quite like that. Perhaps the numbers you said might assume various things such as the width of the eye's field of view, the distance to the image and the size of each character? My experience is that the readability of source code decreases when the lines are very long (more than 160 characters). The point is that the width, excluding leading and trailing white space, is what really matters. Even deeply indented code can be a snap to understand if you don't have to fight artificial line breaks. And we've got a much wider -- and taller! -- space available than we had in the old 80x24 (and 80x1) days. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Sat, 2008-02-23 at 00:55 +1030, David Newall wrote: Bart Van Assche wrote: There is a reason to limit line length: scientific research has shown that readability of regular texts is optimal for a line length between 55 and 65 characters. Putting aside the point that we're talking code, not regular text, I've heard that said before and I don't think it's quite like that. Perhaps the numbers you said might assume various things such as the width of the eye's field of view, the distance to the image and the size of each character? Not in my experience. My experience is that the readability of source code decreases when the lines are very long (more than 160 characters). The point is that the width, excluding leading and trailing white space, is what really matters. Even deeply indented code can be a snap to understand if you don't have to fight artificial line breaks. And we've got a much wider -- and taller! -- space available than we had in the old 80x24 (and 80x1) days. I have 2 24 screens running at 1920x1200 with X forced to 75dpi and use a 8pt Monospace font. (Yes, I can read that from more than 3ft away) Using a fullscreen gvim (without the icons, but with the menu) with 3 vertical splits gives me 4 columns of 113 rows and 95 chars. So, yes, I have the screen estate for very long lines, but I find that long lines require more effort to read (that very much includes leading whitespace). Also, since long lines are rare (and they should be, if you nest too deep you have other issues) accommodating them would waste a lot of screen estate otherwise useful for another column of text. Even with e-mail, I can easily show over 200 characters wide with a large font (say 11pt) but find it harder to read emails that don't nicely wrap at 78. So much so that I often find myself not reading the mail, or restyling it if I find it important enough to read anyway. Please, lets keep the 80 as a guideline, and not trip over the occasional lines that exceed it in good style (read: wrapping them would indeed give uglier code) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Sat, Feb 23, 2008 at 12:55:03AM +1030, David Newall wrote: Bart Van Assche wrote: There is a reason to limit line length: scientific research has shown that readability of regular texts is optimal for a line length between 55 and 65 characters. Putting aside the point that we're talking code, not regular text, I've heard that said before and I don't think it's quite like that. Perhaps the numbers you said might assume various things such as the width of the eye's field of view, the distance to the image and the size of each character? I'm sure all those assumptions are baked-in to the estimate. Yet the fact remains that people's eyes are only so good and most people will be reading at similar distances from the screen. So I don't see any reason to invalidate those assumptions. FWIW, I find reading longer lines to be painful -- it is easier to loose one's place in the text. I would also echo a point Jeff Garzik made elsewhere that it is often beneficial to have multiple windows oppen side-by-side. Longer lines makes it harder to do that in a useful way. Instead the lines either wrap or just trail off the screen. See the output of sdiff for how this limits usefulness. My experience is that the readability of source code decreases when the lines are very long (more than 160 characters). The point is that the width, excluding leading and trailing white space, is what really matters. Even deeply indented code can be a snap to understand if you don't have to fight artificial line breaks. And we've got a much wider -- and taller! -- space available than we had in the old 80x24 (and 80x1) days. I'm not sure deeply indented code is ever a snap to understand. And FWIW, I'd rather deal with artificial line breaks than parameter lists that just stream off the side of the page. The line breaks make long parameters lists easier to digest. I'll sacrifice the occasional odd breakage of a long string. John -- John W. Linville [EMAIL PROTECTED] -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
* Linus Torvalds [EMAIL PROTECTED] wrote: I'm personally of the opinion that a lot of checkpatch fixes are anything but. That mainly concerns fixing overlong lines (where the fixed version is usually worse than the original), but it's been true for some other warnings too. that was certainly the case for the earlier checkpatch releases which treated overlong lines as an error. So here's a quick list of negative and positive aspects of current versions of checkpatch, as i see them. But let me first declare it that when scripts/checkpatch.pl was initially merged last year i immediately ran it over my own files and became a deep sceptic of it. (check the lkml archives, i complained alot about it) Now i've got more than half a year of experience with using checkpatch as an integral part of scheduler maintenance, and we've now got 4 months of experience with using checkpatch in arch/x86 maintenance. Based on this first hand experience, my opinion about checkpatch has changed, rather radically: i now believe that checkpatch is almost as important to the long term health of our kernel development process as BitKeeper/Git turned out to be. If i had to stop using it today, it would be almost as bad of a step backwards to me as if we had to migrate the kernel source code control to CVS. Lets see the Bad Side of checkpatch: 1) checkpatch errors shouldnt be taken too seriously for newly introduced leaf driver code, which code we dont at all know whether we'll be maintaining in any serious manner in the future. Slowing down a submission by requirig it to pass checkpatch is not as clear-cut as it is for core infrastructure and architecture code. It's far more important to get _any_ code to users (as long as it's not outright harmful) than to nitpick about style details. 2) it still has some false positives. (They are quite rare in the latest versions, about 1 out of 100 for code that is already clean. I send them over to Andy whenever i see them, and they get fixed quickly. The false positives were a big annoyance in early checkpatch.pl versions, these days they are not - to me at least.) 3) it's _really_ annoying when sometimes i stumble over some old, crufty piece of code that according to checkpatch is in high need of some good, thorough cleanup - and when i take a look at the code it turns out that the original author of that crap piece of code turns out to be ... me. Those moments can be pretty embarrasing and sobering ;-) The Good Side of checkpatch (and here i'll only list the non-obvious advantages): 1) 90% of the scheduler related checkpatch fixes today you'll never recognize in a commit! The fixes all happen before code is submitted, and the fixes are seemlessly embedded in nice looking patches. (in that sense checkpatch is a bit like lockdep: 90% of the errors they detect wont hit lkml, ever.) 2) you might know that Deja-Vu moment when you look at a new patch that has been submitted to lkml and you have a strange, weird feeling that there's something wrong about the patch. It's totally subconscious, and you take a closer look and a few seconds later you find a real bug in the code. That feeling i believe comes from a fundamental property of how human vision is connected to the human brain: pattern matching. Really good programmers have built a library of patterns of good and bad looking coding practices. If a patch or if a file has a clean _style_, bugs and deeper structural problems often stand out like a sore thumb. But if the code is peppered with random style noise, it's a lot harder (for me at least) to notice real bugs. I can notice bugs in a squeeky clean code base about 5 times easier than in a noisy codebase. This effect alone makes checkpatch indispensible for the scheduler and for arch/x86. Sidenote: i dont really need fancy metrics trying to tell me how good an algorithm _truly_ is (although it certainly would be interesting to have). I can _see_ that at a glance - provided the code follows common kernel practices and a common, consistent style. Checkpatch makes visual code patterns universal and eases the human maintainance work enormously, for a 150+ KLOC subsystem like arch/x86. I'm not distracted (visually and mentally) by the thick fog of small silly details and quirks in coding style. Others might have radar eyes and radar brains, i dont :-) 3) checkpatch also keeps _my_ bugs out of the kernel in an interesting way. I'm sure many of you are like me: i've got weaker moments when i write rather crappy code, and i've got stronger moments when i'm in the flow and can write a few thousand lines of code with nary a hickup. What makes things worse is it's really hard to tell the two apart. It turns out - and this surprised me a
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 04:17:17PM +0100, Peter Zijlstra wrote: Even with e-mail, I can easily show over 200 characters wide with a large font (say 11pt) but find it harder to read emails that don't nicely wrap at 78. So much so that I often find myself not reading the mail, or restyling it if I find it important enough to read anyway. Yes, ditto. And since most of my patch review is done inside mutt... -- John W. Linville [EMAIL PROTECTED] -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 7:54 PM, Ingo Molnar [EMAIL PROTECTED] wrote: If a patch or if a file has a clean _style_, bugs and deeper structural problems often stand out like a sore thumb. But if the code is peppered with random style noise, it's a lot harder (for me at least) to notice real bugs. I can notice bugs in a squeeky clean code base about 5 times easier than in a noisy codebase. This effect alone makes checkpatch indispensible for the scheduler and for arch/x86. I also appreciate style uniformity in kernel code. My (limited) experience with checkpatch is that most checkpatch complaints are easy to resolve. Bart Van Assche. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Ingo Molnar wrote: 2) you might know that Deja-Vu moment when you look at a new patch that has been submitted to lkml and you have a strange, weird feeling that there's something wrong about the patch. It's totally subconscious, and you take a closer look and a few seconds later you find a real bug in the code. That feeling i believe comes from a fundamental property of how human vision is connected to the human brain: pattern matching. Really good programmers have built a library of patterns of good and bad looking coding practices. If a patch or if a file has a clean _style_, bugs and deeper structural problems often stand out like a sore thumb. But if the [...] The best programmers are the ones who have a good eye for details - and that subconsciously extends to style details too. I've yet to see a _single_ example of a good, experienced kernel programmer who writes code that looks absolutely careless and sloppy, but which is top-notch otherwise. (Newbies will make style mistakes a lot more often - and for them checkpatch is a nice and easy experience at reading other people's code and trying to learn the style of the kernel.) [...] 4) there's a psychological effect as well: clean _looking_ code is more attractive to coders to improve upon. Once the code _looks_ clean (mechanically), the people with the real structural cleanups are not far away either. Code that just looks nice is simply more of a pleasure to work with and to improve, so there's a strong psychological relationship between the small, seemingly unimportant details cleanups and the real, structural cleanups. The above deserved to be quoted... just because I agree with all of it so strongly :) Bugs really do hide in ugly code, in part because my brain has been optimized to review clean code. Like everything else in life, one must strike a balance between picking style nits with someone's patch, and making honest criticisms of a patch because said patch is too unclean to be reviewed by anyone. Jeff -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Thu 2008-02-21 14:08:55, Arjan van de Ven wrote: On Thu, 21 Feb 2008 23:01:24 +0200 Adrian Bunk [EMAIL PROTECTED] wrote: [ Linus Added to the To: since I want to hear his opinion on this issue. ] On Thu, Feb 21, 2008 at 12:28:55PM -0800, Roland Dreier wrote: This driver should really have gotten some review before being included in the kernel. Even a simple checkpatch run finds more than 250 stylistic errors (not code bugs but cases where the driver violates the standard code formatting rules of kernel code). Linus has strongly stated that we should merge hardware drivers early, and I agree: although the nes driver clearly needs more work, there's no advantage to users with the hardware in forcing them to wait for 2.6.26 to merge the driver, since they'll just have to patch the grungy code in themselves anyway. And by merging the driver early, we get fixed up for any tree-wide changes and allow janitors to help with the cleanup. Is it really intended to merge drivers without _any_ kind of review? No of course not. I totally agree we should be more agressive in merging drivers earlier. A minimal review needs to happen so for a few things imo 1) That the driver doesn't break the build 2) That the driver has no obvious huge security holes (this is a big deal for unsuspecting users) 3) that there's not an obscene amount of uses deprecated api compiler warnings (since those are annoying for everyone else) 4) that people who don't have the hardware are not negatively affected (say crashes without the hw or so) 5) does not introduce new and ugly user-kernel we'll have problems fixing/removing? Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri 2008-02-22 01:05:26, Krzysztof Halasa wrote: Jeff Garzik [EMAIL PROTECTED] writes: If a driver is full of lines of length 80, that's a problem. I'm not sure. We all have more than 80-chars wide displays for years, don't we? The No. Zaurus is one example, second is small screen where you need big font to keep it readable (x60 on desk). Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 02:20:12PM -0500, Jeff Garzik wrote: Ingo Molnar wrote: 2) you might know that Deja-Vu moment when you look at a new patch that has been submitted to lkml and you have a strange, weird feeling that there's something wrong about the patch. It's totally subconscious, and you take a closer look and a few seconds later you find a real bug in the code. That feeling i believe comes from a fundamental property of how human vision is connected to the human brain: pattern matching. Really good programmers have built a library of patterns of good and bad looking coding practices. If a patch or if a file has a clean _style_, bugs and deeper structural problems often stand out like a sore thumb. But if the [...] The best programmers are the ones who have a good eye for details - and that subconsciously extends to style details too. I've yet to see a _single_ example of a good, experienced kernel programmer who writes code that looks absolutely careless and sloppy, but which is top-notch otherwise. (Newbies will make style mistakes a lot more often - and for them checkpatch is a nice and easy experience at reading other people's code and trying to learn the style of the kernel.) [...] 4) there's a psychological effect as well: clean _looking_ code is more attractive to coders to improve upon. Once the code _looks_ clean (mechanically), the people with the real structural cleanups are not far away either. Code that just looks nice is simply more of a pleasure to work with and to improve, so there's a strong psychological relationship between the small, seemingly unimportant details cleanups and the real, structural cleanups. The above deserved to be quoted... just because I agree with all of it so strongly :) Bugs really do hide in ugly code, in part because my brain has been optimized to review clean code. Like everything else in life, one must strike a balance between picking style nits with someone's patch, and making honest criticisms of a patch because said patch is too unclean to be reviewed by anyone. I totally agree with all of this. checkpatch.pl is a useful tool to use, and is quite handy for helping the kernel code for all of the above reasons. /aol thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Al Viro [EMAIL PROTECTED] writes: IMO the line length overruns make good warnings. Not as in here's a cheap way to get more changesets, but as in that code might have other problems nearby kind of heuristics. Sure, it does. However the human looking at the code is far better at spotting such problems. Machine-generated warnings are great when the machine is actually better than human. Anyway, warnings are one thing and line limit is another. We may raise the limit leaving the 80-chars warning in place. Unless there are too many false positives, of course. -- Krzysztof Halasa -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Pavel Machek [EMAIL PROTECTED] writes: Zaurus is one example, second is small screen where you need big font to keep it readable (x60 on desk). Come on, are you doing Linux kernel development on PDA? -- Krzysztof Halasa -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Linus Torvalds [EMAIL PROTECTED] writes: Will people then try to fake things out by using 4-space indents and then deep indentations will look like just a couple of tabs?) There is no point in faking it as it's only advisory, it's to help the author who should be free to ignore the advice. People upstream won't be fooled by some cheap tab tricks I guess. -- Krzysztof Halasa -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 11:59:35PM +0100, Krzysztof Halasa wrote: Sure - because email is not C code. Actually you don't read C code, word by word, as you read books - do you? If it's decently written - sure, why not? Unfortunately, more common case is somewhere between the writing on the lavatory wall and appartment lease agreement, with several high school essays mixed in... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
Peter Zijlstra [EMAIL PROTECTED] writes: So, yes, I have the screen estate for very long lines, but I find that long lines require more effort to read (that very much includes leading whitespace). Also, since long lines are rare (and they should be, if you nest too deep you have other issues) accommodating them would waste a lot of screen estate otherwise useful for another column of text. Either they are rare and you can wrap them and still use 80 columns, or it turns out they are not so rare and you may want to use wider windows (not necessarily 132 but perhaps 100). I think the question isn't if they are rare or not, or if people have 3 * 1920 pixels/line or just 1280. The question is: is the code more readable with hard limit equal to 80 characters, or maybe is it better to limit code block complexity instead, and let the maximum number of those small pictures in a line alone? (Limiting at 132 would have technical sense IMHO). Better code readability = less bugs without any additional effort. Even with e-mail, I can easily show over 200 characters wide with a large font (say 11pt) but find it harder to read emails that don't nicely wrap at 78. Sure - because email is not C code. Actually you don't read C code, word by word, as you read books - do you? -- Krzysztof Halasa -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Thu, Feb 21, 2008 at 7:13 PM, Linus Torvalds
<[EMAIL PROTECTED]> wrote:
> So I'd be happier with warnings about deep indentation (but how do you
> count it? Will people then try to fake things out by using 4-space indents
> and then "deep" indentations will look like just a couple of tabs?)
I suspect that 90% of the cases that people really care about would
get caught successfully just by counting brace depth.
ie, by looking at { { {} {} {{{}{}}} } } I bet you can tell me which
section should have been pulled out into a separate routine.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
Linus Torvalds <[EMAIL PROTECTED]> writes: > So I'd be happier with warnings about deep indentation (but how do you > count it? Will people then try to fake things out by using 4-space indents > and then "deep" indentations will look like just a couple of tabs?) and > against complex expressions (ie "if ((a = xyz()) == NULL) .." should just > be split up into "a = xyz(); if (!a) ..", but there are sometimes reasons > for those things too! Deep indentation should be fairly easy, given that you already have rules in place that says "Tabs are 8 characters". So if you find a line that begins with more than say 4 SP, you can flag that as already bogus (i.e. "does not indent with HT"), more than 8 SP definitely so. I'll leave harder "complex expressions" to sparse experts ;-), -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 03:23:45AM +0100, Krzysztof Halasa wrote: > Al Viro <[EMAIL PROTECTED]> writes: > > > ... if your style is lousy. I agree that situation with printks is > > not normal in that respect and I certainly have no love for the > > checkpatch nonsense, but pressure to keep the fucking nesting depth > > low is a Good Thing(tm). > > Indeed. Unfortunately it is orthogonal to the line length limit. Not quite. Add such things as choice of sane identifiers. And sane use of local variables, while we are at it - things like twenty lines of foobar[(index + 1) % BLAH]->spork.vomit[12]->field_name = ; with the only difference in the field_name, except for one line where we have a typo and see 11 instead of intended 12, are responsible for quite a few of such overruns. IMO the line length overruns make good warnings. Not as in "here's a cheap way to get more changesets", but as in "that code might have other problems nearby" kind of heuristics. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri, 22 Feb 2008, Al Viro wrote: > > ... if your style is lousy. I agree that situation with printks is > not normal in that respect and I certainly have no love for the > checkpatch nonsense, but pressure to keep the fucking nesting depth > low is a Good Thing(tm). I do agree, but that has little to do with line length *directly*. IOW, I'd personally be happier with a checkpatch that calculated "complexity" and indentation over line length. There is definitely a correlation there: there is no question that complex lines with deep indentation tend to be long. So yes, "long lines are correlated with bad code" is certainly true to some degree. But sometimes lines are long just because it's a function call with multiple parameters, and it's just three levels indented, and it had a string there too. It may be long, but it's not complex, and keeping it on one line actually makes it much easier to visually parse (and grep for, for that matter). So I'd be happier with warnings about deep indentation (but how do you count it? Will people then try to fake things out by using 4-space indents and then "deep" indentations will look like just a couple of tabs?) and against complex expressions (ie "if ((a = xyz()) == NULL) .." should just be split up into "a = xyz(); if (!a) ..", but there are sometimes reasons for those things too! Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Al Viro <[EMAIL PROTECTED]> writes: > ... if your style is lousy. I agree that situation with printks is > not normal in that respect and I certainly have no love for the > checkpatch nonsense, but pressure to keep the fucking nesting depth > low is a Good Thing(tm). Indeed. Unfortunately it is orthogonal to the line length limit. We should limit the nesting level, though I think there is no universally good value. What is good for one case (a function with a short multi-level if/for/etc) is bad for another (a long switch() where any added complexity makes it unparseable). So I think it just have to meet the author's and reviewers' taste. We already depend on this. -- Krzysztof Halasa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 12:16:45PM +1030, David Newall wrote: > Krzysztof Halasa wrote: > > Linus Torvalds <[EMAIL PROTECTED]> writes: > >> I'm personally of the opinion that a lot of checkpatch "fixes" are > >> anything but. That mainly concerns fixing overlong lines > >> > > > > Perhaps we should increase line length limit, 132 should be fine. > > Especially useful with long printk() lines and long arithmetic > > expressions. > > > > > Yes; or even longer. 80 characters might have made sense on a screen > when the alternative was 80 characters on a punched card, but on a > modern computer it's very restrictive. That's especially true with the > deep indents that you quickly get in C ... if your style is lousy. I agree that situation with printks is not normal in that respect and I certainly have no love for the checkpatch nonsense, but pressure to keep the fucking nesting depth low is a Good Thing(tm). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Jeff Garzik <[EMAIL PROTECTED]> writes: > Every time this discussion comes up, people point out that it remains > highly common to open multiple 80-column terminal windows, making the > 80-column limit still highly relevant in modern times. I guess only because of the limit :-) Raise the limit, terminal windows will follow. I'm using 80-column windows, too. -- Krzysztof Halasa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Krzysztof Halasa wrote: > Linus Torvalds <[EMAIL PROTECTED]> writes: >> I'm personally of the opinion that a lot of checkpatch "fixes" are >> anything but. That mainly concerns fixing overlong lines >> > > Perhaps we should increase line length limit, 132 should be fine. > Especially useful with long printk() lines and long arithmetic > expressions. > Yes; or even longer. 80 characters might have made sense on a screen when the alternative was 80 characters on a punched card, but on a modern computer it's very restrictive. That's especially true with the deep indents that you quickly get in C. Even short lines often need to be split when you put a few tabs in front of them, and that makes comprehension that bit harder, not to mention looks ugly. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Thu, Feb 21, 2008 at 01:30:37PM -0800, Greg KH wrote:
> On Thu, Feb 21, 2008 at 11:01:24PM +0200, Adrian Bunk wrote:
> >
> > BTW: Greg, you are Cc'ed for your joke in [3]...
>
> > [3] http://lkml.org/lkml/2008/2/12/427
>
> That was not a joke, I ment it. Do you have proof that the majority of
> patches going into the kernel tree are not reviewed by at least 2
> people?
>...
I don't see any way for getting a proof in any direction, but no matter
how many SOB lines a patch has my impression is that usually at a
maximum the one person who applies a patch reviews it ("review" as in
"understands the code in question well and reviews the patch line for
line").
Sometimes there's even simply noone who could a patch at all, e.g. I'm
not sure whether there is anyone at all who would be able to review a
patch by Sam fiddling with kbuild internals.
How many lines of code get changed in the kernel per day?
And we should have for each changed line two people who are both
experienced enough in this area of the kernel and who have the time to
review this line?
Even one of our best maintained subsystems has commits that contain
bugs like
+ if ((!tid_agg_rx->reorder_buf) && net_ratelimit()) {
+ printk(KERN_ERR "can not allocate reordering buffer "
+ "to tid %d\n", tid);
+ goto end;
+ }
> thanks,
>
> greg k-h
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Krzysztof Halasa wrote: Jeff Garzik <[EMAIL PROTECTED]> writes: If a driver is full of lines of length >80, that's a problem. I'm not sure. We all have more than 80-chars wide displays for years, don't we? The Every time this discussion comes up, people point out that it remains highly common to open multiple 80-column terminal windows, making the 80-column limit still highly relevant in modern times. The problem is [...] code which is too complex and which may sometimes have too many levels of indentation. Quite true. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Thu, Feb 21, 2008 at 11:31:44PM +, Alan Cox wrote:
> On Fri, 22 Feb 2008 00:38:14 +0100
> Krzysztof Halasa <[EMAIL PROTECTED]> wrote:
>
> > Linus Torvalds <[EMAIL PROTECTED]> writes:
> >
> > > I'm personally of the opinion that a lot of checkpatch "fixes" are
> > > anything but. That mainly concerns fixing overlong lines
> >
> > Perhaps we should increase line length limit, 132 should be fine.
> > Especially useful with long printk() lines and long arithmetic
> > expressions.
>
> Agreed. The fact I'm having to fix bugs introduced by incorrect printk
> wrapping confirms that for printk strings at least it is overzealous.
>
> I'm all for it complaining about
>
> printk(KERN_FOO "<90 chars>", foo, bar + 37);
>
> type bits when the foo, bar should be underneath to be visible but for
> straight quoted text too long it should not warn and try to get the text
> folded.
I think it should warn, but people have to be aware of the following:
- checkpatch errors are for stuff that really has to be fixed
- checkpatch warnings are for stuff that should be looked at
- the goal is not 0 checkpatch warnings but readable and bugfree code
A nice property of checkpatch is that it encourages to look closer at
code like the following (it warns about the volatile):
if (!netif_queue_stopped(netdev)) {
netif_stop_queue(netdev);
barrier();
if ((volatile
u16)nesnic->sq_tail)+(nesnic->sq_size*2))-nesnic->sq_head) & (nesnic->sq_size -
1)) != 1) {
netif_start_queue(netdev);
goto sq_no_longer_full;
}
}
> Alan
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Jeff Garzik <[EMAIL PROTECTED]> writes: > If a driver is full of lines of length >80, that's a problem. I'm not sure. We all have more than 80-chars wide displays for years, don't we? The problem is not the number of characters but code which is too complex and which may sometimes have too many levels of indentation. Unfortunately expressing code complexity in terms of line lengths doesn't seem to work at all. The 80-chars limit harms development, it makes the code less readable, sometimes far less readable. I think we should increase length limit to 132 for the whole kernel code. Obviously printk() _output_ etc. should stay at 80. -- Krzysztof Halasa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Here are some PCI patches against your 2.6.25-rc2 git tree. They are a collection of PCI quirk additions, build fixes, and some PCI hotplug fixes. Please pull from: master.kernel.org:/pub/scm/linux/kernel/git/gregkh/pci-2.6.git/ The full patches will be sent to the linux-pci mailing list, if anyone wants to see them. thanks, greg k-h - arch/x86/kernel/acpi/boot.c|3 +- arch/x86/pci/irq.c |4 ++ drivers/pci/hotplug/acpiphp_core.c |6 +++ drivers/pci/hotplug/acpiphp_ibm.c | 33 --- drivers/pci/pci-acpi.c |1 + drivers/pci/proc.c |2 +- drivers/pci/quirks.c | 79 --- drivers/pci/setup-bus.c|6 +-- drivers/pcmcia/i82092.c|7 +++- include/linux/pci_ids.h|6 +++ 10 files changed, 117 insertions(+), 30 deletions(-) --- Andrew Morton (2): PCI: drivers/pcmcia/i82092.c: fix up after pci_bus_region changes PCI: fix up setup-bus.c #ifdef Crane Cai (1): PCI: AMD SATA IDE mode quirk Gary Hade (1): PCI: hotplug: acpiphp_ibm: Remove get device information Jason Gaston (2): PCI: pci_ids: patch for Intel ICH10 DeviceID's PCI: irq: patch for Intel ICH10 DeviceID's Kenji Kaneshige (1): PCI: Fix wrong reference counter check for proc_dir_entry Peer Chen (1): PCI: quirks: set 'En' bit of MSI Mapping for devices onHT-based nvidia platform Randy Dunlap (1): PCI: kernel-doc: fix pci-acpi warning Yinghai Lu (1): PCI: don't load acpi_php when acpi is disabled -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Thu, Feb 21, 2008 at 11:01:24PM +0200, Adrian Bunk wrote: > > BTW: Greg, you are Cc'ed for your joke in [3]... > [3] http://lkml.org/lkml/2008/2/12/427 That was not a joke, I ment it. Do you have proof that the majority of patches going into the kernel tree are not reviewed by at least 2 people? Now they might not be 2 people that you personally like/agree with, but that's a totally different topic... And I'm with Linus on this one, it's much easier to work on driver fixes together with others, when they are in the kernel tree. Although I do like the checkpatch.pl script, it has helped me in making it easier to clean up some vendor-provided drivers recently, finding some obvious coding style issues that I had missed the first pass through. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Thu, 21 Feb 2008, Adrian Bunk wrote: > > Is it really intended to merge drivers without _any_ kind of review? I'd really rather have the driver merged, and then *other* people can send patches! The thing is, that's what merging really means - people can work on it sanely together. Before it's merged, it's a lot harder for people to work on it unless they are really serious about that driver, so before merging, the janitorial kind of things seldom happen. So yes, I really do believe that we should merge drivers in particular a lot more aggressively. I'd like to see *testing* feedback, in order to not merge drivers that simply don't work well enough, but anything else? I suspect other feedback is as likely to cause problems as it is to fix things. > This driver even lacks a basic "please fix the > 250 checkpatch errors" [1] > and similar low hanging fruits that could easily be spotted and then > fixed by the submitter within a short amount of time. Quite frankly, I've several times been *this* close (holds up fingers so you can't even see between them) to just remove checkpatch entirely. I'm personally of the opinion that a lot of checkpatch "fixes" are anything but. That mainly concerns fixing overlong lines (where the "fixed" version is usually worse than the original), but it's been true for some other warnings too. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
> Is it really intended to merge drivers without _any_ kind of review? > > This driver even lacks a basic "please fix the > 250 checkpatch errors" [1] > and similar low hanging fruits that could easily be spotted and then > fixed by the submitter within a short amount of time. Just to be clear, this driver was reviewed. Many issues were found, and many were fixed while others are being worked on. It's a judgement call when to merge things, but in this case given the good engagement from the vendor, I didn't see anything to be gained by delaying the merge. - R. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Merging of completely unreviewed drivers
[ Linus Added to the To: since I want to hear his opinion on this issue. ] On Thu, Feb 21, 2008 at 12:28:55PM -0800, Roland Dreier wrote: > > This driver should really have gotten some review before being included > > in the kernel. > > > Even a simple checkpatch run finds more than > 250 stylistic errors > > (not code bugs but cases where the driver violates the standard code > > formatting rules of kernel code). > > Linus has strongly stated that we should merge hardware drivers early, > and I agree: although the nes driver clearly needs more work, there's > no advantage to users with the hardware in forcing them to wait for > 2.6.26 to merge the driver, since they'll just have to patch the > grungy code in themselves anyway. And by merging the driver early, we > get fixed up for any tree-wide changes and allow janitors to help with > the cleanup. Is it really intended to merge drivers without _any_ kind of review? This driver even lacks a basic "please fix the > 250 checkpatch errors" [1] and similar low hanging fruits that could easily be spotted and then fixed by the submitter within a short amount of time. I see the point that it might make sense to not prevent the merging of drivers infinitely when they have some hard-to-fix issues, but was this really meant as an excuse for maintainers to no longer any review of what they merge at all? > (By the way, the code is not that pretty but it a lot closer to > upstream style than most driver submissions) >... There might be worse code being submitted, but when looking at what gets merged into Linus' tree this driver beats all other drivers I remember in both number of stylistic problems and bugs. [2] > - R. cu Adrian BTW: Greg, you are Cc'ed for your joke in [3]... [1] not to mention the > 2000 checkpatch warnings [2] as already said, that's not meant against the driver submitter I'm complaining about the complete lack of review that would have brought this driver into shape [3] http://lkml.org/lkml/2008/2/12/427 -- "Is there not promise of rain?" Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. "Only a promise," Lao Er said. Pearl S. Buck - Dragon Seed -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Merging of completely unreviewed drivers
[ Linus Added to the To: since I want to hear his opinion on this issue. ] On Thu, Feb 21, 2008 at 12:28:55PM -0800, Roland Dreier wrote: This driver should really have gotten some review before being included in the kernel. Even a simple checkpatch run finds more than 250 stylistic errors (not code bugs but cases where the driver violates the standard code formatting rules of kernel code). Linus has strongly stated that we should merge hardware drivers early, and I agree: although the nes driver clearly needs more work, there's no advantage to users with the hardware in forcing them to wait for 2.6.26 to merge the driver, since they'll just have to patch the grungy code in themselves anyway. And by merging the driver early, we get fixed up for any tree-wide changes and allow janitors to help with the cleanup. Is it really intended to merge drivers without _any_ kind of review? This driver even lacks a basic please fix the 250 checkpatch errors [1] and similar low hanging fruits that could easily be spotted and then fixed by the submitter within a short amount of time. I see the point that it might make sense to not prevent the merging of drivers infinitely when they have some hard-to-fix issues, but was this really meant as an excuse for maintainers to no longer any review of what they merge at all? (By the way, the code is not that pretty but it a lot closer to upstream style than most driver submissions) ... There might be worse code being submitted, but when looking at what gets merged into Linus' tree this driver beats all other drivers I remember in both number of stylistic problems and bugs. [2] - R. cu Adrian BTW: Greg, you are Cc'ed for your joke in [3]... [1] not to mention the 2000 checkpatch warnings [2] as already said, that's not meant against the driver submitter I'm complaining about the complete lack of review that would have brought this driver into shape [3] http://lkml.org/lkml/2008/2/12/427 -- Is there not promise of rain? Ling Tan asked suddenly out of the darkness. There had been need of rain for many days. Only a promise, Lao Er said. Pearl S. Buck - Dragon Seed -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Is it really intended to merge drivers without _any_ kind of review? This driver even lacks a basic please fix the 250 checkpatch errors [1] and similar low hanging fruits that could easily be spotted and then fixed by the submitter within a short amount of time. Just to be clear, this driver was reviewed. Many issues were found, and many were fixed while others are being worked on. It's a judgement call when to merge things, but in this case given the good engagement from the vendor, I didn't see anything to be gained by delaying the merge. - R. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Thu, 21 Feb 2008, Adrian Bunk wrote: Is it really intended to merge drivers without _any_ kind of review? I'd really rather have the driver merged, and then *other* people can send patches! The thing is, that's what merging really means - people can work on it sanely together. Before it's merged, it's a lot harder for people to work on it unless they are really serious about that driver, so before merging, the janitorial kind of things seldom happen. So yes, I really do believe that we should merge drivers in particular a lot more aggressively. I'd like to see *testing* feedback, in order to not merge drivers that simply don't work well enough, but anything else? I suspect other feedback is as likely to cause problems as it is to fix things. This driver even lacks a basic please fix the 250 checkpatch errors [1] and similar low hanging fruits that could easily be spotted and then fixed by the submitter within a short amount of time. Quite frankly, I've several times been *this* close (holds up fingers so you can't even see between them) to just remove checkpatch entirely. I'm personally of the opinion that a lot of checkpatch fixes are anything but. That mainly concerns fixing overlong lines (where the fixed version is usually worse than the original), but it's been true for some other warnings too. Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Thu, Feb 21, 2008 at 11:01:24PM +0200, Adrian Bunk wrote: BTW: Greg, you are Cc'ed for your joke in [3]... [3] http://lkml.org/lkml/2008/2/12/427 That was not a joke, I ment it. Do you have proof that the majority of patches going into the kernel tree are not reviewed by at least 2 people? Now they might not be 2 people that you personally like/agree with, but that's a totally different topic... And I'm with Linus on this one, it's much easier to work on driver fixes together with others, when they are in the kernel tree. Although I do like the checkpatch.pl script, it has helped me in making it easier to clean up some vendor-provided drivers recently, finding some obvious coding style issues that I had missed the first pass through. thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Here are some PCI patches against your 2.6.25-rc2 git tree. They are a collection of PCI quirk additions, build fixes, and some PCI hotplug fixes. Please pull from: master.kernel.org:/pub/scm/linux/kernel/git/gregkh/pci-2.6.git/ The full patches will be sent to the linux-pci mailing list, if anyone wants to see them. thanks, greg k-h - arch/x86/kernel/acpi/boot.c|3 +- arch/x86/pci/irq.c |4 ++ drivers/pci/hotplug/acpiphp_core.c |6 +++ drivers/pci/hotplug/acpiphp_ibm.c | 33 --- drivers/pci/pci-acpi.c |1 + drivers/pci/proc.c |2 +- drivers/pci/quirks.c | 79 --- drivers/pci/setup-bus.c|6 +-- drivers/pcmcia/i82092.c|7 +++- include/linux/pci_ids.h|6 +++ 10 files changed, 117 insertions(+), 30 deletions(-) --- Andrew Morton (2): PCI: drivers/pcmcia/i82092.c: fix up after pci_bus_region changes PCI: fix up setup-bus.c #ifdef Crane Cai (1): PCI: AMD SATA IDE mode quirk Gary Hade (1): PCI: hotplug: acpiphp_ibm: Remove get device information Jason Gaston (2): PCI: pci_ids: patch for Intel ICH10 DeviceID's PCI: irq: patch for Intel ICH10 DeviceID's Kenji Kaneshige (1): PCI: Fix wrong reference counter check for proc_dir_entry Peer Chen (1): PCI: quirks: set 'En' bit of MSI Mapping for devices onHT-based nvidia platform Randy Dunlap (1): PCI: kernel-doc: fix pci-acpi warning Yinghai Lu (1): PCI: don't load acpi_php when acpi is disabled -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Jeff Garzik [EMAIL PROTECTED] writes: If a driver is full of lines of length 80, that's a problem. I'm not sure. We all have more than 80-chars wide displays for years, don't we? The problem is not the number of characters but code which is too complex and which may sometimes have too many levels of indentation. Unfortunately expressing code complexity in terms of line lengths doesn't seem to work at all. The 80-chars limit harms development, it makes the code less readable, sometimes far less readable. I think we should increase length limit to 132 for the whole kernel code. Obviously printk() _output_ etc. should stay at 80. -- Krzysztof Halasa -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Thu, Feb 21, 2008 at 11:31:44PM +, Alan Cox wrote:
On Fri, 22 Feb 2008 00:38:14 +0100
Krzysztof Halasa [EMAIL PROTECTED] wrote:
Linus Torvalds [EMAIL PROTECTED] writes:
I'm personally of the opinion that a lot of checkpatch fixes are
anything but. That mainly concerns fixing overlong lines
Perhaps we should increase line length limit, 132 should be fine.
Especially useful with long printk() lines and long arithmetic
expressions.
Agreed. The fact I'm having to fix bugs introduced by incorrect printk
wrapping confirms that for printk strings at least it is overzealous.
I'm all for it complaining about
printk(KERN_FOO 90 chars, foo, bar + 37);
type bits when the foo, bar should be underneath to be visible but for
straight quoted text too long it should not warn and try to get the text
folded.
I think it should warn, but people have to be aware of the following:
- checkpatch errors are for stuff that really has to be fixed
- checkpatch warnings are for stuff that should be looked at
- the goal is not 0 checkpatch warnings but readable and bugfree code
A nice property of checkpatch is that it encourages to look closer at
code like the following (it warns about the volatile):
if (!netif_queue_stopped(netdev)) {
netif_stop_queue(netdev);
barrier();
if ((volatile
u16)nesnic-sq_tail)+(nesnic-sq_size*2))-nesnic-sq_head) (nesnic-sq_size -
1)) != 1) {
netif_start_queue(netdev);
goto sq_no_longer_full;
}
}
Alan
cu
Adrian
--
Is there not promise of rain? Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
Only a promise, Lao Er said.
Pearl S. Buck - Dragon Seed
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Krzysztof Halasa wrote: Jeff Garzik [EMAIL PROTECTED] writes: If a driver is full of lines of length 80, that's a problem. I'm not sure. We all have more than 80-chars wide displays for years, don't we? The Every time this discussion comes up, people point out that it remains highly common to open multiple 80-column terminal windows, making the 80-column limit still highly relevant in modern times. The problem is [...] code which is too complex and which may sometimes have too many levels of indentation. Quite true. Jeff -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Thu, Feb 21, 2008 at 01:30:37PM -0800, Greg KH wrote:
On Thu, Feb 21, 2008 at 11:01:24PM +0200, Adrian Bunk wrote:
BTW: Greg, you are Cc'ed for your joke in [3]...
[3] http://lkml.org/lkml/2008/2/12/427
That was not a joke, I ment it. Do you have proof that the majority of
patches going into the kernel tree are not reviewed by at least 2
people?
...
I don't see any way for getting a proof in any direction, but no matter
how many SOB lines a patch has my impression is that usually at a
maximum the one person who applies a patch reviews it (review as in
understands the code in question well and reviews the patch line for
line).
Sometimes there's even simply noone who could a patch at all, e.g. I'm
not sure whether there is anyone at all who would be able to review a
patch by Sam fiddling with kbuild internals.
How many lines of code get changed in the kernel per day?
And we should have for each changed line two people who are both
experienced enough in this area of the kernel and who have the time to
review this line?
Even one of our best maintained subsystems has commits that contain
bugs like
+ if ((!tid_agg_rx-reorder_buf) net_ratelimit()) {
+ printk(KERN_ERR can not allocate reordering buffer
+ to tid %d\n, tid);
+ goto end;
+ }
thanks,
greg k-h
cu
Adrian
--
Is there not promise of rain? Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
Only a promise, Lao Er said.
Pearl S. Buck - Dragon Seed
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Krzysztof Halasa wrote: Linus Torvalds [EMAIL PROTECTED] writes: I'm personally of the opinion that a lot of checkpatch fixes are anything but. That mainly concerns fixing overlong lines Perhaps we should increase line length limit, 132 should be fine. Especially useful with long printk() lines and long arithmetic expressions. Yes; or even longer. 80 characters might have made sense on a screen when the alternative was 80 characters on a punched card, but on a modern computer it's very restrictive. That's especially true with the deep indents that you quickly get in C. Even short lines often need to be split when you put a few tabs in front of them, and that makes comprehension that bit harder, not to mention looks ugly. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Jeff Garzik [EMAIL PROTECTED] writes: Every time this discussion comes up, people point out that it remains highly common to open multiple 80-column terminal windows, making the 80-column limit still highly relevant in modern times. I guess only because of the limit :-) Raise the limit, terminal windows will follow. I'm using 80-column windows, too. -- Krzysztof Halasa -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 12:16:45PM +1030, David Newall wrote: Krzysztof Halasa wrote: Linus Torvalds [EMAIL PROTECTED] writes: I'm personally of the opinion that a lot of checkpatch fixes are anything but. That mainly concerns fixing overlong lines Perhaps we should increase line length limit, 132 should be fine. Especially useful with long printk() lines and long arithmetic expressions. Yes; or even longer. 80 characters might have made sense on a screen when the alternative was 80 characters on a punched card, but on a modern computer it's very restrictive. That's especially true with the deep indents that you quickly get in C ... if your style is lousy. I agree that situation with printks is not normal in that respect and I certainly have no love for the checkpatch nonsense, but pressure to keep the fucking nesting depth low is a Good Thing(tm). -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
Al Viro [EMAIL PROTECTED] writes: ... if your style is lousy. I agree that situation with printks is not normal in that respect and I certainly have no love for the checkpatch nonsense, but pressure to keep the fucking nesting depth low is a Good Thing(tm). Indeed. Unfortunately it is orthogonal to the line length limit. We should limit the nesting level, though I think there is no universally good value. What is good for one case (a function with a short multi-level if/for/etc) is bad for another (a long switch() where any added complexity makes it unparseable). So I think it just have to meet the author's and reviewers' taste. We already depend on this. -- Krzysztof Halasa -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri, Feb 22, 2008 at 03:23:45AM +0100, Krzysztof Halasa wrote: Al Viro [EMAIL PROTECTED] writes: ... if your style is lousy. I agree that situation with printks is not normal in that respect and I certainly have no love for the checkpatch nonsense, but pressure to keep the fucking nesting depth low is a Good Thing(tm). Indeed. Unfortunately it is orthogonal to the line length limit. Not quite. Add such things as choice of sane identifiers. And sane use of local variables, while we are at it - things like twenty lines of foobar[(index + 1) % BLAH]-spork.vomit[12]-field_name = expr; with the only difference in the field_name, except for one line where we have a typo and see 11 instead of intended 12, are responsible for quite a few of such overruns. IMO the line length overruns make good warnings. Not as in here's a cheap way to get more changesets, but as in that code might have other problems nearby kind of heuristics. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Fri, 22 Feb 2008, Al Viro wrote: ... if your style is lousy. I agree that situation with printks is not normal in that respect and I certainly have no love for the checkpatch nonsense, but pressure to keep the fucking nesting depth low is a Good Thing(tm). I do agree, but that has little to do with line length *directly*. IOW, I'd personally be happier with a checkpatch that calculated complexity and indentation over line length. There is definitely a correlation there: there is no question that complex lines with deep indentation tend to be long. So yes, long lines are correlated with bad code is certainly true to some degree. But sometimes lines are long just because it's a function call with multiple parameters, and it's just three levels indented, and it had a string there too. It may be long, but it's not complex, and keeping it on one line actually makes it much easier to visually parse (and grep for, for that matter). So I'd be happier with warnings about deep indentation (but how do you count it? Will people then try to fake things out by using 4-space indents and then deep indentations will look like just a couple of tabs?) and against complex expressions (ie if ((a = xyz()) == NULL) .. should just be split up into a = xyz(); if (!a) .., but there are sometimes reasons for those things too! Linus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ofa-general] Re: Merging of completely unreviewed drivers
Linus Torvalds [EMAIL PROTECTED] writes: So I'd be happier with warnings about deep indentation (but how do you count it? Will people then try to fake things out by using 4-space indents and then deep indentations will look like just a couple of tabs?) and against complex expressions (ie if ((a = xyz()) == NULL) .. should just be split up into a = xyz(); if (!a) .., but there are sometimes reasons for those things too! Deep indentation should be fairly easy, given that you already have rules in place that says Tabs are 8 characters. So if you find a line that begins with more than say 4 SP, you can flag that as already bogus (i.e. does not indent with HT), more than 8 SP definitely so. I'll leave harder complex expressions to sparse experts ;-), -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Merging of completely unreviewed drivers
On Thu, Feb 21, 2008 at 7:13 PM, Linus Torvalds
[EMAIL PROTECTED] wrote:
So I'd be happier with warnings about deep indentation (but how do you
count it? Will people then try to fake things out by using 4-space indents
and then deep indentations will look like just a couple of tabs?)
I suspect that 90% of the cases that people really care about would
get caught successfully just by counting brace depth.
ie, by looking at { { {} {} {{{}{}}} } } I bet you can tell me which
section should have been pulled out into a separate routine.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
