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

Julia Lawall
2023-03-07T07:50:24Z

[-- 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
> >
> >
> >
>
>
>