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

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:
> >
> > When a value has been tested for NULL in an expression, a
> > second NULL test on the same value in another expression
> > is unnecessary when the value has not been assigned NULL.
> >
> > Remove unnecessary duplicate NULL tests on the same value that
> > has previously been NULL tested.
> >
> > Found by Coccinelle.
> 
> ...
> 
> > +                       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.

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.

regards,
dan carpenter