Re: [PATCH] staging: axis-fifo: alignment should match opening parenthesis in axis-fifo.c

[Julia Lawall <julia.lawall@inria.fr>]

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



On Tue, 7 Mar 2023, Khadija wrote:

> Hey Julia! Thank you for the feedback. I will make the following changes and
> resend the patch:
> 1. Correct the patch description that is right under the subject line (make
> it precise and imperative) and make sure that it does not have more than 70
> characters per line.
> 2. Adjust all the arguments of wait_event_interruptible_timeout so that they
> are lined up. All of them should begin right under ( .

The problem here is that the ( is really far to the right.  My opinion is
that the position of the second argument (ie the first one that is on a
line of its own) is ok in this case.  So you can leave that one where it
is and line up the other one.

julia

> Kindly let me know if I have understood it
> right.[EIu7f6EdNusD0UITKM3h?rid=EIu7f6EdNusD0UITKM3h]
>
> On Tue, Mar 7, 2023 at 2:08 AM Julia Lawall <julia.lawall@inria.fr> wrote:
>
>
>       On Tue, 7 Mar 2023, Khadija Kamran wrote:
>
>       > In file drivers/staging/axis-fifo/axis-fifo.c the alignment
>       did not match the opening parenthesis. So, a few tabs were added
>       to match the alignment to exactly where the parenthesis started.
>
>       Hello Khadija,
>
>       Thanks for plunging in and being the first participant!
>
>       However, there are a number of issues with the proposed patch.
>
>       1.  The log message should be at most around 70 characters
>       wide.  You have
>       one long line.
>
>       2.  The log message should be written in the imperative. 
>       Instead of "a
>       few tabs were added", ay "add a few tabs".
>
>       3.  I'm not sure that it is worth creating a very long line to
>       respect the
>       rule about (.  On the other hand, the way the code is written at
>       the
>       moment seems to be very misleading, because the third argument
>       to
>       wait_event_interruptible_timeout is written as though it is the
>       second
>       argument to ioread32.  So you can adjust the argument list of
>       wait_event_interruptible_timeout so that at least all of the
>       arguments
>       that are not on the same line as the function call are lined up.
>
>       julia
>
>       >
>       > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
>       > ---
>       >  drivers/staging/axis-fifo/axis-fifo.c | 2 +-
>       >  1 file changed, 1 insertion(+), 1 deletion(-)
>       >
>       > diff --git a/drivers/staging/axis-fifo/axis-fifo.c
>       b/drivers/staging/axis-fifo/axis-fifo.c
>       > index dfd2b357f484..6e959224add0 100644
>       > --- a/drivers/staging/axis-fifo/axis-fifo.c
>       > +++ b/drivers/staging/axis-fifo/axis-fifo.c
>       > @@ -383,7 +383,7 @@ static ssize_t axis_fifo_read(struct file
>       *f, char __user *buf,
>       >                */
>       >               mutex_lock(&fifo->read_lock);
>       >               ret =
>       wait_event_interruptible_timeout(fifo->read_queue,
>       > -                     ioread32(fifo->base_addr +
>       XLLF_RDFO_OFFSET),
>       > +                                                   
>       ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
>       >                                (read_timeout >= 0) ?
>       >                                 msecs_to_jiffies(read_timeout)
>       :
>       >                                 MAX_SCHEDULE_TIMEOUT);
>       > --
>       > 2.34.1
>       >
>       >
>       >
>
>
>