Re: [v3 1/1] staging: rtl8723bs: Prevent duplicate NULL tests on a value

On Fri, Apr 4, 2025 at 12:06 PM Dan Carpenter <[email protected]> wrote:
> On Fri, Apr 04, 2025 at 10:53:22AM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 4, 2025 at 3:03 AM Abraham Samuel Adekunle
> > <[email protected]> wrote:

...

> > > +                       psta->sta_xmitpriv.txseq_tid[pattrib->priority] &= 0xFFF;
> >
> > > +                                       psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> > > +                                               (tx_seq + 1) & 0xfff;
> >
> > > +                                       psta->BA_starting_seqctrl[pattrib->priority & 0x0f] =
> > > +                                               (pattrib->seqnum + 1) % 4096;
> >
> > Logically it's obvious that you need to align all cases to have
> > consistent approach.
> > Besides that the commit message should mention this change. Something like this
> > "While at it, convert '& 0xfff' cases to use modulo operator and
> > decimal number to make the upper limit visible and clear what the
> > semantic of it is."
>
> No, I'm sorry but that's really against the rules in drivers/staging.
> Don't mix unrelated changes into a patch.  It needs to be done as a
> separate patch if we're going to do that.
>
> To be honest, I don't even want people fixing line length issues or
> adding spaces.  I would have accepted small white space changes but I
> prefered the v2 version of this patch.  Once you start changing
> "& 0xfff" to "% 4096" that's not white space and it must be done
> in a separate patch. I use a script to review white space patches
> because I'm always nervous someone will slip something malicious
> into 100+ lines of reformated code.  It's really fast to review
> patches with my script but once people start mixing things in then
> it's a headache for me.

Separate patch is even better, indeed.

> Also if the change accidentally introduces a bug, I want it to be a
> one liner change and not something hidden inside a giant reformat.

The noisy {] have no point to be left. Now I'm curious, what do you
propose here?

-- 
With Best Regards,
Andy Shevchenko