Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[David Woodhouse]

On Wed, 2006-01-18 at 16:52 -0800, akpm@osdl.org wrote:
> -                       memcpy(&current->saved_sigmask, &sigsaved, sizeof(sigsaved));
> +                       memcpy(&current->saved_sigmask, &sigsaved,
> +                                       sizeof(sigsaved));

I still object to this.

You justified it on the basis that some people have editors which will
wrap the original version onto a second line and make it look ugly...
yet your 'fix' is to wrap it onto a second line and make it look ugly
for _all_ of us, not just for those using crap editors. I really don't
see the overall benefit.

-- 
dwmw2

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

Hi David,

On Thu, 19 Jan 2006 16:21:59 +1100 David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Wed, 2006-01-18 at 16:52 -0800, akpm@osdl.org wrote:
> > -                       memcpy(&current->saved_sigmask, &sigsaved, sizeof(sigsaved));
> > +                       memcpy(&current->saved_sigmask, &sigsaved,
> > +                                       sizeof(sigsaved));
> 
> I still object to this.
> 
> You justified it on the basis that some people have editors which will
> wrap the original version onto a second line and make it look ugly...
> yet your 'fix' is to wrap it onto a second line and make it look ugly
> for _all_ of us, not just for those using crap editors. I really don't
> see the overall benefit.

Documentation/CodingStyle says:

The limit on the length of lines is 80 columns and this is a hard limit.

Statements longer than 80 columns will be broken into sensible chunks.
Descendants are always substantially shorter than the parent and are placed
substantially to the right. The same applies to function headers with a long
argument list. Long strings are as well broken into shorter strings.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[David Woodhouse]

On Thu, 2006-01-19 at 17:17 +1100, Stephen Rothwell wrote:
> The limit on the length of lines is 80 columns and this is a hard limit.
> 
> Statements longer than 80 columns will be broken into sensible chunks.
> Descendants are always substantially shorter than the parent and are placed
> substantially to the right. The same applies to function headers with a long
> argument list. Long strings are as well broken into shorter strings.

We can submit patches for the cases where the guidelines listed in
Documentation/CodingStyle diverge from common sense. 

In _some_ cases, the text which one might put after the 80th column is
actually important to the flow of the program and really should be put
back into the 'normal' text area. That's fair enough -- I'm not arguing
that we should leave 80-column text areas behind altogether.

But in this case it's just the length argument to a memcpy, and there's
no real information there. Similarly, people have recently been observed
to start wrapping the strings in debugging printks onto a second line
gratuitously. In _those_ cases, it really is counter-productive -- it's
_fine_ if that text is off the right-hand side of the screen.

If your editor wraps it onto the next line, then that sucks -- but at
least it only sucks for _you_, and you wouldn't really benefit by the
proposed 'fix' anyway, because the proposed 'fix' is just to wrap it so
that it sucks for _all_ of us.

-- 
dwmw2

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[Andrew Morton]

Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Documentation/CodingStyle says:
> 
>  The limit on the length of lines is 80 columns and this is a hard limit.
> 
>  Statements longer than 80 columns will be broken into sensible chunks.
>  Descendants are always substantially shorter than the parent and are placed
>  substantially to the right. The same applies to function headers with a long
>  argument list. Long strings are as well broken into shorter strings.

That's pretty stern.

I'd be happy with a 96-col standard, or 100 or whatever - it's more
convenient and I use twin 20" guns.  But other people have different
hardware constraints and different work practices, so they want 80 cols. 
If we're going to get down and change the standard then OK, let's have that
bunfight.  But while there's a standard we should stick to it so we don't
screw over the people who like to use standard-sized xterms.

And yes, some editors can do sideways-scrolling to make wider-than-80
acceptable in an 80-col window.  But other people's setups don't do that,
and the cost to those people of wrappy code is higher than the cost of
looking at standardly-laid-out code to fancy-editor users.

So the lowest common denominator wins, because they hurt more than anyone
else if we go outside 80-cols.  I use 80-col xterms precisely for this
reason: so that the code which goes in will look OK to those users.

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[David Woodhouse]

On Wed, 2006-01-18 at 22:30 -0800, Andrew Morton wrote:
> So the lowest common denominator wins, because they hurt more than
> anyone else if we go outside 80-cols.  I use 80-col xterms precisely
> for this reason: so that the code which goes in will look OK to those
> users.

That would make some sense if it weren't for the fact that the snippet
of patch I just showed doesn't actually improve the situation for the
lowest common denominator -- it only makes it worse for the rest of us.

Put real meaningful stuff within 80 columns by all means -- but please
allow lines to be longer than that if they're just 'fluff'. 

-- 
dwmw2

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[David S. Miller]

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Thu, 19 Jan 2006 17:17:08 +1100

> Documentation/CodingStyle says:
> 
> The limit on the length of lines is 80 columns and this is a hard limit.
> 
> Statements longer than 80 columns will be broken into sensible chunks.
> Descendants are always substantially shorter than the parent and are placed
> substantially to the right. The same applies to function headers with a long
> argument list. Long strings are as well broken into shorter strings.

I wish there were an exception for function prototypes and definitions.
Why?  So grep actually works.

Hmmm, what args does function X take?  Let's try this:

bash$ git grep X

Oops, the args went past 80 columns and was split up, so I only
get the first few in the grep output.

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[Trond Myklebust]

On Wed, 2006-01-18 at 22:36 -0800, David S. Miller wrote:
> I wish there were an exception for function prototypes and definitions.
> Why?  So grep actually works.

Seconded! Even people with antediluvian editors will find it useful to
be able to grep for function prototypes and/or definitions in order to
figure out argument definitions, argument orders, etc.

Cheers,
  Trond

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[Andrew Morton]

Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
>
> On Wed, 2006-01-18 at 22:36 -0800, David S. Miller wrote:
>  > I wish there were an exception for function prototypes and definitions.
>  > Why?  So grep actually works.
> 
>  Seconded!

hm.  Why not use $EDITOR's ctags/etags/etc?

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[David Woodhouse]

On Wed, 2006-01-18 at 23:02 -0800, Andrew Morton wrote:
> hm.  Why not use $EDITOR's ctags/etags/etc?

Some editors don't have them -- and certainly not for the git trees
pulled from upstream which I keep pristine.

-- 
dwmw2

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[David S. Miller]

From: Andrew Morton <akpm@osdl.org>
Date: Wed, 18 Jan 2006 23:02:27 -0800

> Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> >
> > On Wed, 2006-01-18 at 22:36 -0800, David S. Miller wrote:
> >  > I wish there were an exception for function prototypes and definitions.
> >  > Why?  So grep actually works.
> > 
> >  Seconded!
> 
> hm.  Why not use $EDITOR's ctags/etags/etc?

Why rebuild that every time the tree changes when we have
grep and "git grep" which sees updates automatically? :-)

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[James Morris]

On Wed, 18 Jan 2006, David S. Miller wrote:

> I wish there were an exception for function prototypes and definitions.
> Why?  So grep actually works.
> 
> Hmmm, what args does function X take?  Let's try this:
> 
> bash$ git grep X
> 
> Oops, the args went past 80 columns and was split up, so I only
> get the first few in the grep output.

Linus already made this exception, some time ago on lkml, IIRC). I think 
it was 120 cols for functions.


- James
-- 
James Morris
<jmorris@namei.org>

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 453 bytes --]

On Wed, 18 Jan 2006 22:36:29 -0800 (PST) "David S. Miller" <davem@davemloft.net> wrote:
>
> Hmmm, what args does function X take?  Let's try this:
> 
> bash$ git grep X
> 
> Oops, the args went past 80 columns and was split up, so I only
> get the first few in the grep output.

git grep -A2 X

:-)  (now I am just being silly :-))

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[Alan Cox]

On Iau, 2006-01-19 at 17:17 +1100, Stephen Rothwell wrote:
> Documentation/CodingStyle says:
> 
> The limit on the length of lines is 80 columns and this is a hard limit.

Its so hard nobody follows it because in many places the results as Dave
correctly points out are just stupid.

Linux 2.6.16-rc1
	Number of files matching *.[c|h]	: 15732
	Number with lines exceeding 80 columns	: 6931
	As a percentage				: 44%

Fix the CodingStyle document instead

Alan

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[Jens Axboe]

On Thu, Jan 19 2006, Alan Cox wrote:
> On Iau, 2006-01-19 at 17:17 +1100, Stephen Rothwell wrote:
> > Documentation/CodingStyle says:
> > 
> > The limit on the length of lines is 80 columns and this is a hard limit.
> 
> Its so hard nobody follows it because in many places the results as Dave
> correctly points out are just stupid.
> 
> Linux 2.6.16-rc1
> 	Number of files matching *.[c|h]	: 15732
> 	Number with lines exceeding 80 columns	: 6931
> 	As a percentage				: 44%
> 
> Fix the CodingStyle document instead

89% of all statistics are bogus, right? In those files, how many lines
are correctly wrapped even if some are > 80 chars?

I think the CodingStyle suggestion of 80 chars is just fine. I try to
stay within that, and I mostly succeed. The occasional over-the-line is
far better than advocation >> 80 chars per line imho.

-- 
Jens Axboe

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[David Woodhouse]

On Thu, 2006-01-19 at 16:59 +0100, Jens Axboe wrote:
> I think the CodingStyle suggestion of 80 chars is just fine. I try to
> stay within that, and I mostly succeed. The occasional over-the-line is
> far better than advocation >> 80 chars per line imho.

I agree. It's that "occasional over-the-line" which Andrew is mucking
about with in the patch which started this thread; the case where it
makes _sense_ to let it go over 80 characters rather than wrapping it.

-- 
dwmw2

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[Andrew Morton]

David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Thu, 2006-01-19 at 16:59 +0100, Jens Axboe wrote:
> > I think the CodingStyle suggestion of 80 chars is just fine. I try to
> > stay within that, and I mostly succeed. The occasional over-the-line is
> > far better than advocation >> 80 chars per line imho.
> 
> I agree. It's that "occasional over-the-line" which Andrew is mucking
> about with in the patch which started this thread; the case where it
> makes _sense_ to let it go over 80 characters rather than wrapping it.
> 

Oh crap.  The damn thing wraps into column _1_ and gets tangled up with
ifdef statements, function definitions and other things which _should_ go
in column one.

It .looks.  .like.  .crap.  to many other people, and saying random stupid
wrong things doesn't alter that very simple fact.

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[David Woodhouse]

On Fri, 2006-01-20 at 00:44 -0800, Andrew Morton wrote:
> Oh crap.  The damn thing wraps into column _1_ and gets tangled up with
> ifdef statements, function definitions and other things which _should_ go
> in column one.

It does that only for people with editors which wrap stuff like that
into column 1. Those people (which includes myself on some occasions)
are _used_ to seeing stuff like that in column 1, so it's natural. And
it's text which is of little importance; not something which has much
relevance to the code flow.

> It .looks.  .like.  .crap.  to many other people, and saying random stupid
> wrong things doesn't alter that very simple fact.

No, it looks like crap for _some_ people. And making it look like crap
for _everyone_, which is what your patch does, doesn't alter that fact
either.

It's a simple memcpy(dest, src, len), and the length is almost entirely
superfluous -- we could almost get away with '*dest = *src' here. It
lives on a single line and is messy any other way, and you want to muck
about with it just because there are some poor sods out there for whom
it would look _slightly_ better if you make it look like crap for the
rest of us.

I'm not advocating a blanket removal of the 80-character limit;
important things should always be within 80 columns. But this is fluff;
leave it where it is, please.

-- 
dwmw2

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[Eric Dumazet]

David Woodhouse a écrit :
> On Fri, 2006-01-20 at 00:44 -0800, Andrew Morton wrote:
>> Oh crap.  The damn thing wraps into column _1_ and gets tangled up with
>> ifdef statements, function definitions and other things which _should_ go
>> in column one.
> 
> It does that only for people with editors which wrap stuff like that
> into column 1. Those people (which includes myself on some occasions)
> are _used_ to seeing stuff like that in column 1, so it's natural. And
> it's text which is of little importance; not something which has much
> relevance to the code flow.
> 
>> It .looks.  .like.  .crap.  to many other people, and saying random stupid
>> wrong things doesn't alter that very simple fact.
> 
> No, it looks like crap for _some_ people. And making it look like crap
> for _everyone_, which is what your patch does, doesn't alter that fact
> either.

David,

Some readers of linux kernel sources are blind.
They use a kind of terminal that 'displays' a single line of 80 'characters' 
(or even 40) called a 'Braille Display'

This kind of terminal is very expensive, and I think the 80 column one is the 
most you can get (price : about 7000$).

I am ok to be a litle bit upset by this 80 limitation that looks odd on my 
1000$ 24" display, but reminds me the fact that some human people are different.

So please don't count me as part of your _everyone_.

Thank you
Eric

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[David Woodhouse]

On Fri, 2006-01-20 at 11:01 +0100, Eric Dumazet wrote:
> Some readers of linux kernel sources are blind.
> They use a kind of terminal that 'displays' a single line of 80 'characters' 
> (or even 40) called a 'Braille Display'
> 
> This kind of terminal is very expensive, and I think the 80 column one is the 
> most you can get (price : about 7000$).

Yes, I've seen them. What's your point? A user with a braille terminal
can also 'see' that it's just a memcpy of the signal set, and doesn't
really need to scroll over to see the length argument to the memcpy,
except in very exceptional circumstances. The code flow is perfectly
understandable without doing so.

> I am ok to be a litle bit upset by this 80 limitation that looks odd on my 
> 1000$ 24" display, but reminds me the fact that some human people are different.
> 
> So please don't count me as part of your _everyone_.

I count your theoretical blind person above as part of 'everyone'. By
gratuitously moving the 'fluff' onto a new line, he gets a whole line
taken up by it when he scrolls down. If it had stayed where I put it, it
wouldn't be getting in his way.

-- 
dwmw2

Re: - add-pselect-ppoll-system-call-implementation-tidy.patch removed from -mm tree

[Adrian Bunk]

On Fri, Jan 20, 2006 at 09:59:54PM +1300, David Woodhouse wrote:
> On Fri, 2006-01-20 at 00:44 -0800, Andrew Morton wrote:
> > Oh crap.  The damn thing wraps into column _1_ and gets tangled up with
> > ifdef statements, function definitions and other things which _should_ go
> > in column one.
> 
> It does that only for people with editors which wrap stuff like that
> into column 1. Those people (which includes myself on some occasions)
> are _used_ to seeing stuff like that in column 1, so it's natural. And
> it's text which is of little importance; not something which has much
> relevance to the code flow.
>...

Patches with lines > 80 are not easily readable in 80 column xterms no 
matter whether the lines are wrapped or not.

> dwmw2

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