lkml.org 
[lkml]   [2009]   [Jul]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] vt: add an event interface

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > Well i noticed such details in your final commits that go upstream
> > as well - you dont appear to be making full use of the patch quality
> > tools we have.
>
> Quite often deliberately. A lot of the tty patches keep whatever
> style they are patching. You'll then see a single big patch to
> clean the style of the entire file up instead of creating the
> horrible mishmashes of styles found in some of the code.

That's a really broken method IMO, as you basically allow crap (and
get used to allowing crap) instead of just saying: "no crap from me
from today on, ever".

Your method leads to stuff like this in a recent commit:

| commit a6614999e800cf3a134ce93ea46ef837e3c0e76e
| Author: Alan Cox <alan@redhat.com>
| Date: Fri Jan 2 13:46:50 2009 +0000
|
| tty: Introduce some close helpers for ports

+ if( tty->count == 1 && port->count != 1) {
+ printk(KERN_WARNING
+ "tty_port_close_start: tty->count = 1 port count = %d.\n",
+ port->count);
+ port->count = 1;
+ }
+ if (--port->count < 0) {
+ printk(KERN_WARNING "tty_port_close_start: count = %d\n",
+ port->count);
+ port->count = 0;
+ }
Look at how the first branch does 'if( ' while the second one does
the proper 'if ('. It literally hurts the eye - and if it does not
hurt yours it better should ;-)
There is absolutely no justification for stuff like that. It is not
about 'preserving the existing style' - it's inconsistent style in
the same hunk.

Also note the inconsistent printk-ing lines, mutiliated by line
warps. The use of pr_warning() would solve it:

+ if (tty->count == 1 && port->count != 1) {
+ pr_warning("tty_port_close_start: tty->count = 1 port count = %d.\n", port->count);
+ port->count = 1;
+ }
+ if (--port->count < 0) {
+ pr_warning("tty_port_close_start: count = %d\n", port->count);
+ port->count = 0;
+ }
'Allow crap now, we'll fix it later' is a bad policy IMHO. New code
(or old code moved into a new spot) added should always be nice.

Note, there are occasional bogus checkpatch warnings and borderline
cases (as with any tool - for example it will emit a col-80 warning
about my pr_warning() example above and that warning should be
ignored), where checkpatch should be ignored - but this is not one
of them.

Ingo


\
 
 \ /
  Last update: 2009-07-03 11:57    [from the cache]
©2003-2011 Jasper Spaans