Re: [PATCH v9 2/6] staging: vt6655: split long code lines in s_uGetRTSCTSDuration

[Tanju Brunostar]

On Sat, Oct 29, 2022 at 8:47 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Oct 28, 2022 at 11:23:23PM +0000, Tanjuate Brunostar wrote:
> > Increase code visibility by splitting long lines of code in the
> > function: s_uGetRTSCTSDuration
> >
> > Signed-off-by: Tanjuate Brunostar <tanjubrunostar0@xxxxxxxxx>
> > ---
> >  drivers/staging/vt6655/rxtx.c | 108 ++++++++++++++++++++++++----------
> >  1 file changed, 76 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
> > index 7eb7c6eb5cf0..8e56a7ee8035 100644
> > --- a/drivers/staging/vt6655/rxtx.c
> > +++ b/drivers/staging/vt6655/rxtx.c
> > @@ -186,20 +186,29 @@ static __le16 get_rtscts_time(struct vnt_private *priv,
> >
> >       data_time = bb_get_frame_time(priv->preamble_type, pkt_type, frame_length, current_rate);
> >       if (rts_rsvtype == 0) { /* RTSTxRrvTime_bb */
> > -             rts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 20, priv->byTopCCKBasicRate);
> > -             ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopCCKBasicRate);
> > +             rts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 20,
> > +                                          priv->byTopCCKBasicRate);
> > +             ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14,
> > +                                          priv->byTopCCKBasicRate);
>
> While I understand the feeling of "let's fix this warning by wrapping
> the line!" type of solution, overall, it's NOT the correct thing to do.
>
> Remember, coding style changes are to be done to make code easier to
> read and understand, not harder.  The original code here is easier to
> read, and you made it harder to understand.
>
> The "best" solution for this will be to fix up the line length by virtue
> of fixing up the incorrect variable names.  Here is the original lines:
>
>                 rts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 20, priv->byTopCCKBasicRate);
>                 ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopCCKBasicRate);
>
> but if you were to fix up just 1 function and one variable name, look at
> what happens and checkpatch is happy with it:
>
>                 rts_time = get_frame_time(priv->preamble_type, pkt_type, 20, priv->top_basic_rate);
>                 ack_time = get_frame_time(priv->preamble_type, pkt_type, 14, priv->top_basic_rate);
>
> Or even better:
>                 type = priv->preamble_type;
>                 rate = priv->top_basic_rate;
>                 rts_time = get_frame_time(type, pkt_type, 20, rate);
>                 ack_time = get_frame_time(type, pkt_type, 14, rate);
>
> Look, no line-wrapping and isn't that easier to understand?  The second
> version here might even produce smaller compiled code overall, making it
> a even better solution.
>
> So step back, revisit this whole series with the goal of overall making
> the code better and easier to review.  Don't create a series just with
> the short-term goal of making checkpatch quiet.
>
> Hope this helps,
>
> greg k-h
It does help. Thank you.