Re: [patch cr 2/4] [RFC] syscalls, x86: Add __NR_kcmp syscall v7

Ingo Molnar
2012-02-03T09:52:52Z

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 3 Feb 2012 10:09:29 +0100 Ingo Molnar <mingo@elte.hu> wrote:
>
> >
> > * Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> >
> > > > > + get_random_bytes(&cookies[i][j],
> > > > > + sizeof(cookies[i][j]));
> > > >
> > > > ugly line break.
> > > >
> > >
> > > Why? Looks pretty good to me. But sure I'll change it.
> >
> > It's ugly because it serves no purpose other than pacifying
> > checkpatch and makes the code *uglier*.
>
> No it doesn't. For 80-col displays the code is *already
> wrapped*. And that wrapping to column 0 is vastly worse than
> the above.

Have you actually checked how this actual line would look like
in an 80 cols terminal, if not broken up? I have, it's exactly
80 cols so it looks just fine.

( It was probably broken up when it was longer and then left
this way - making things permanently worse not just by the
linebreak but also by the unnecessary curly braces around the
inner loop. )

But more importantly, even if the line was genuinely longer, how
many people are looking at things in an 80-col display? By my
experience, from looking at what kinds of terminals kernel
people use, it's below 1%. (I was one of the last ones holding
out because text consoles are so much faster than just about any
usable xterm app - but I switched to a larger terminal some two
years ago.)

Shouldnt't we concentrate on the 99% case which gets uglified by
the systematic linebreaks?

Also, there are clearly cases where breaking the line
intelligently improves things. Such as:

+ /* An example of output and arguments */
+ printf("pid1: %6d pid2: %6d FD: %2d FILES: %2d VM: %2d FS: %2d "
+ "SIGHAND: %2d IO: %2d SYSVSEM: %2d INV: %2d\n",
+ pid1, pid2,
+ sys_kcmp(pid1, pid2, KCMP_FILE, fd1, fd2),
+ sys_kcmp(pid1, pid2, KCMP_FILES, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_VM, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_FS, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_SIGHAND, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_IO, 0, 0),
+ sys_kcmp(pid1, pid2, KCMP_SYSVSEM, 0, 0),
+
+ /* This one should fail */
+ sys_kcmp(pid1, pid2, KCMP_TYPES + 1, 0, 0));

this is vastly more readable because the arguments are lined up
vertically not just at the beginning but nicely tabulated along
the way.

Oh, and note that

Breaking lines is a tool that should be used on a case by case
basis, not a hard limit.

> If we want to increase the standard to (say) 96 cols then
> fine, I'd be happy with that. But until we do that we should
> not create such a gruesome mess for those who use 80 cols.

The kernel has *already* become a gruesome mess for 80 col users
long ago. That was the main reason why I stopped using 80 col
terminals two years ago ...

So lets stop the pretense.

> > It's a disease. When checkpatch tells you "this line is too
> > long" then consider it a code cleanliness warning!
>
> Well yes, if it can be fixed by other means then great.

Yes it can.

Thanks,

Ingo