lkml.org 
[lkml]   [2009]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/5] ide: Add support for TRIM
    Hello.

    Matthew Wilcox wrote:

    >>> +static int idedisk_prepare_discard(struct request_queue *q, struct
    >>>request *rq,
    >>>+ struct bio *bio)

    >> Weird indentation.

    > Not at all. New line, so tab the next line over as far as it will go
    > without falling off the 80-column limit. It's fairly common.

    > To quote Documentation/CodingStyle:

    > 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. The
    > only exception to this is where exceeding 80 columns significantly increases
    > readability and does not hide information.

    That's all clear, it's just that the second line was overly indented, at
    least to my taste. :-)

    >>>+{
    >>>+ ide_task_t *task;

    >> This patch is already obsolete as 'ide_task_t' is gone. Use 'struct
    >>ide_cmd' instead.

    > Thanks, fixed.

    >>>+ unsigned size;
    >>>+ struct page *page = alloc_page(GFP_KERNEL);

    >> Missing empty line after the declaration block...

    > Empty line not necessary.

    Usually it's there.

    >>>+ task->tf_flags = IDE_TFLAG_LBA48 | IDE_TFLAG_OUT_HOB |
    >>>+ IDE_TFLAG_OUT_TF | IDE_TFLAG_OUT_DEVICE |

    >> The last 3 flags are going to be obsoleted too...

    > So if I remove them today, the command will still work?

    s/obsoleted/renamed and moved to another other field/ -- I'm going to
    submit a patchset refactoring 'struct ide_cmd and 'struct ide-taskfile' at last...

    MBR, Sergei


    \
     
     \ /
      Last update: 2009-04-02 18:41    [W:0.027 / U:34.440 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site