This patch fixes an off-by-one spotted by the Coverity checker.
Signed-off-by: Adrian Bunk <bunk@kernel.org>
---
 drivers/infiniband/hw/nes/nes_verbs.c |    2 +-

 1 file changed, 1 insertion(+), 1 deletion(-)
--- linux-2.6/drivers/infiniband/hw/nes/nes_verbs.c.old	2008-02-20 00:20:47.000000000 +0200

+++ linux-2.6/drivers/infiniband/hw/nes/nes_verbs.c	2008-02-20 00:21:09.000000000 +0200

@@ -916,33 +916,33 @@ static struct ib_pd *nes_alloc_pd(struct

 	if (!nespd) {

 		nes_free_resource(nesadapter, nesadapter->allocated_pds, pd_num);

 		return ERR_PTR(-ENOMEM);

 	}
 	nes_debug(NES_DBG_PD, "Allocating PD (%p) for ib device %s\n",

 			nespd, nesvnic->nesibdev->ibdev.name);
 	nespd->pd_id = (pd_num << (PAGE_SHIFT-12)) + nesadapter->base_pd;
 	if (context) {

 		nesucontext = to_nesucontext(context);

 		nespd->mmap_db_index = find_next_zero_bit(nesucontext->allocated_doorbells,

 				NES_MAX_USER_DB_REGIONS, nesucontext->first_free_db);

 		nes_debug(NES_DBG_PD, "find_first_zero_biton doorbells returned %u, mapping pd_id %u.\n",

 				nespd->mmap_db_index, nespd->pd_id);

-		if (nespd->mmap_db_index > NES_MAX_USER_DB_REGIONS) {

+		if (nespd->mmap_db_index >= NES_MAX_USER_DB_REGIONS) {

 			nes_debug(NES_DBG_PD, "mmap_db_index > MAX\n");

 			nes_free_resource(nesadapter, nesadapter->allocated_pds, pd_num);

 			kfree(nespd);

 			return ERR_PTR(-ENOMEM);

 		}
 		uresp.pd_id = nespd->pd_id;

 		uresp.mmap_db_index = nespd->mmap_db_index;

 		if (ib_copy_to_udata(udata, &uresp, sizeof (struct nes_alloc_pd_resp))) {

 			nes_free_resource(nesadapter, nesadapter->allocated_pds, pd_num);

 			kfree(nespd);

 			return ERR_PTR(-EFAULT);

 		}
 		set_bit(nespd->mmap_db_index, nesucontext->allocated_doorbells);

 		nesucontext->mmap_db_index[nespd->mmap_db_index] = nespd->pd_id;
--

Thanks, this is already upstream as 51af33e8

--

No, 51af33e8 was for a similar same bug 400 lines below this bug...
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
--

> No, 51af33e8 was for a similar same bug 400 lines below this bug...
Heh, sorry.
Glenn -- please review Adrian's patches and let me know which ones are

good to apply.

--

Sweeping through them right now.  Should have something for you

tonight.
Glenn

--

I went ahead and created a patch series and attributed Adrian

for the patches of his I liked.  There were a couple that

I tweaked.  Wasn't sure if all the hunks would apply nicely

after that if we mixed and matched his and mine, hence the series.
Hope that's okay.  Should I have gotten his ack for the ones

I rewrote?  The fixes were pretty small so I figured they didn't

really need more review.
The patch series is on the way...
Glenn

--

Looking at the patches what you did seems OK.
But regarding "review" I have a different criticism directed at Roland:
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).
And I'm not talking about the > 2000 checkpatch warnings that are mostly

about too long lines (which should arguably also be fixed).
And many more issues that could have been foung during a review.

E.g. when you look at 3/8 from this series the code

        if (!cm_node)

                return -EINVAL;

        new_send = kzalloc(sizeof(*new_send), GFP_ATOMIC);

        if (!new_send)

                return -1;

doesn't look good since the -1 should most likely better be something

like -ENOMEM (I haven't checked whether you can immediately change it

at this specific place).
And these are just comments from someone with zero knowledge about

InfiniBand, but I'd expect InfiniBand-specifig bugs might be found

before they hit users if an InfiniBand maintainer would review the

complete driver.
Note that this is not meant as a criticism against Glenn - it's

normal that submitted code contains bugs, but a code review can help to 
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
--

> 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.
(By the way, the code is not that pretty but it a lot closer to

upstream style than most driver submissions)
 > And these are just comments from someone with zero knowledge about

 > InfiniBand, but I'd expect InfiniBand-specifig bugs might be found

 > before they hit users if an InfiniBand maintainer would review the

 > complete driver.
Just for the record, although this driver is under drivers/infiniband,

it is actually for a device that does iWARP/10 Gb ethernet.  At some

point we may want to rename drivers/infiniband to drivers/rdma, but so

far the churn hasn't seemed worth it for what is basically a cosmetic

issue.
 - R.

--

[ Linus Added to the To: since I want to hear his opinion on this issue. ]
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 
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 
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
--

On Thu, 21 Feb 2008 23:01:24 +0200
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)
beyond that.. that's what EXPERIMENTAL is for

(joking; lets not open that can of fish)

--

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

--

FWIW, my general guidelines for merging drivers in my areas are:
1) it's not fugly
2) it has an active maintainer who responds to feedback
I tend to think it is NOT in the best interests of Linux users, for us

to merge vendor-fugly drivers with many layers of OS wrappers and

similar obfuscation.
But similarly...  I merge drivers long before our SCSI maintainer will,

and I value "it works" above stupid checkpatch warnings.
	Jeff
--

I was not talking about checkpatch warnings.
I'm talking about checkpatch errors for code like

						if ((page_count!=0)&&(page_count<<12)-(region->offset&(4096-1))>=region->length)
I have to accept that Linus prefers to have the driver merged first and

let janitors make the code readable in subsequent patches, but if GNU

indent wasn't unable to properly cope with the fact that this driver has

over 2000 lines that are over 80 characters long I'd simply run this 
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
--

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

--

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;
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
--

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 
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

--

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 co...
[ message continues ]

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
--

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

--

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.

--

Perhaps we should increase line length limit, 132 should be fine.

Especially useful with long printk() lines and long arithmetic

expressions.

--

Krzysztof Halasa

--

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.

--

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
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 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
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

linville@tuxdriver.com

--

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)
--

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
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

--

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...

--

Yes, ditto.  And since most of my patch review is done inside mutt...
--

John W. Linville

linville@tuxdriver.com

--

... 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

--

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

--

On Thu, Feb 21, 2008 at 7:13 PM, Linus Torvalds
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;

}
--

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;

	}

}
--

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 ;-),
--

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.
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
--

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

--

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.

--

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

--

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.

--

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

--

On Fri, 22 Feb 2008 00:38:14 +0100
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.
Alan

--

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;

			}
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
--

I think checkpatch is useful, but I've agreed from the beginning that

the line length complaint is completely silly.
If a driver is full of lines of length >80, that's a problem.  If it's

just a few, that's more of a developer decision based on the individual

line of code.
	Jeff
--

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

--

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

--

Come on, are you doing Linux kernel development on PDA?

--

Krzysztof Halasa

--

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

--

I take it the "sometimes" is the key word :-)

--

Krzysztof Halasa

--

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 ...
[ message continues ]

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

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.)

--

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, 
No, that's not it at all. We don't indent 'case' because it matches with 
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

--

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
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

--

Then it's a silly example to raise in a serious discussion of this type.

--

On Fri, 22 Feb 2008 01:05:26 +0100
Even a vt132 serial terminal or later can do 132. Decades not years.

--

Every time this discussion comes up, people point out that it remains

highly common to open multiple 80-column terminal windows, making the 
Quite true.
	Jeff
--

I guess only because of the limit :-)

Raise the limit, terminal windows will follow.

I'm using 80-column windows, too.

--

Krzysztof Halasa

--

Speaking of driver, could authors please comment all those barrier()

calls and remove trailing "return;" at the end of void functions.

--

--

checkpatch would never allow a patch to patch checkpatch.

--

Sorry, I'm not touching it with an eigthy six foot pole. :^)

--

Drat, you beat me to that response.....  :)
--

> 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.

--