LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To: Joe Perches <joe@perches.com>, Greg KH <gregkh@linuxfoundation.org>
Cc: Michael Straube <straube.linux@gmail.com>,
	Larry.Finger@lwfinger.net, phil@philpotter.co.uk,
	martin@kaiser.cx, linux-staging@lists.linux.dev,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: r8188eu: refactor rtw_is_cckrates{only}_included()
Date: Wed, 18 Aug 2021 08:23:18 +0200	[thread overview]
Message-ID: <2509261.CYLKgzzBkz@localhost.localdomain> (raw)
In-Reply-To: <YRwEyrSLTPl/KY9t@kroah.com>

On Tuesday, August 17, 2021 8:49:46 PM CEST Greg KH wrote:
> On Tue, Aug 17, 2021 at 11:36:09AM -0700, Joe Perches wrote:
> > On Tue, 2021-08-17 at 19:57 +0200, Greg KH wrote:
> > > On Mon, Aug 16, 2021 at 09:31:25PM +0200, Michael Straube wrote:
> > > > Refactor functions rtw_is_cckrates_included() and
> > > > rtw_is_cckratesonly_included(). Add new helper function rtw_is_cckrate()
> > > > that allows to make the code more compact. Improves readability and
> > > > slightly reduces object file size. Change the return type to bool to
> > > > reflect that the functions return boolean values.
> > []
> > > > diff --git a/drivers/staging/r8188eu/core/rtw_ieee80211.c b/drivers/staging/r8188eu/core/rtw_ieee80211.c
> > []
> > > > +bool rtw_is_cckratesonly_included(u8 *rate)
> > > >  {
> > > > -	u32 i = 0;
> > > > +	u8 r;
> > > >  
> > > > 
> > > > -	while (rate[i] != 0) {
> > > > -		if  ((((rate[i]) & 0x7f) != 2) && (((rate[i]) & 0x7f) != 4) &&
> > > > -		     (((rate[i]) & 0x7f) != 11)  && (((rate[i]) & 0x7f) != 22))
> > > > +	while ((r = *rate++)) {
> > > 
> > > Ick, no.
> > > 
> > > While it might be fun to play with pointers like this, trying to
> > > determine the precedence issues involved with reading from, and then
> > > incrementing the pointer like this is crazy.
> > > 
> > > The original was obvious as to how it was walking through the array.
> > 
> > It's sad to believe *ptr++ is not obvious to you as it's very commonly
> > used in the kernel sources (over 10,000 instances).
> 
> There's lots of sad things in life :(
> 
Dear Greg,

Please reconsider this issue, I mean it. Let me explain why...

Obviously neither Joe or all the people who knows how much you've given to
Linux during these latest two decades (or is it more?) believes that you have 
any problem with operator precedence :-)

Said that, since operator precedence is one of the first topic that every developer
learn in a course on C and that expressions like *ptr++ are used everywhere in
the kernel you are sending a dangerous message...

It looks like you don't trust people here to be able to do anything more than 
trivial clean-ups. If someone here at linux-staging is not able to understand 
the precedence of operators, please stand up and speak!

We here at linux-staging are not class B developers (compared to A class 
developers of other subsystems). For sure, most of us are newcomers with
less experience than other developers who don't choose to work with
drivers/staging, but you should not prevent us from getting experience and 
using common techniques that are perfectly fine in other areas of Linux.

Thanks for your attention and your precious time,

Fabio
 




  parent reply	other threads:[~2021-08-18  6:23 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-16 19:31 Michael Straube
2021-08-16 20:09 ` Fabio M. De Francesco
2021-08-17 17:57 ` Greg KH
2021-08-17 18:36   ` Joe Perches
2021-08-17 18:49     ` Greg KH
2021-08-17 18:59       ` Joe Perches
2021-08-18  6:23       ` Fabio M. De Francesco [this message]
2021-08-18  6:33         ` Greg KH

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2509261.CYLKgzzBkz@localhost.localdomain \
    --to=fmdefrancesco@gmail.com \
    --cc=Larry.Finger@lwfinger.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=joe@perches.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=martin@kaiser.cx \
    --cc=phil@philpotter.co.uk \
    --cc=straube.linux@gmail.com \
    --subject='Re: [PATCH] staging: r8188eu: refactor rtw_is_cckrates{only}_included()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).