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    [W:0.023 / U:59.812 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site