Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Tue, 17 Oct 2023 07:21:13 +0300
From:   Dan Carpenter <dan.carpenter@linaro.org>
To:     Przemek Kitszel <przemyslaw.kitszel@intel.com>
Cc:     Gilbert Adikankwu <gilbertadikankwu@gmail.com>,
        Nam Cao <namcao@linutronix.de>, outreachy@lists.linux.dev,
        manishc@marvell.com, GR-Linux-NIC-Dev@marvell.com,
        coiby.xu@gmail.com, gregkh@linuxfoundation.org,
        netdev@vger.kernel.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: qlge: Add bool type to qlge_idc_wait()
Message-ID: <32b0e468-6943-431c-9f71-68fc41727914@kadam.mountain>
References: <ZSoxLxs45bIuBrHg@gilbert-PC>
 <20231014065813.mQvFyjWb@linutronix.de>
 <20231014071423.UgDor1v0@linutronix.de>
 <ZSpIPippZFtMw2aG@gilbert-PC>
 <78f321db-bc09-06e8-b4ef-ac56ab91e187@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <78f321db-bc09-06e8-b4ef-ac56ab91e187@intel.com>
Precedence: bulk

On Mon, Oct 16, 2023 at 04:19:09PM +0200, Przemek Kitszel wrote:
> On 10/14/23 09:50, Gilbert Adikankwu wrote:
> > On Sat, Oct 14, 2023 at 09:14:23AM +0200, Nam Cao wrote:
> > > On Sat, Oct 14, 2023 at 08:58:13AM +0200, Nam Cao wrote:
> > > > On Sat, Oct 14, 2023 at 07:11:59AM +0100, Gilbert Adikankwu wrote:
> > > > > Reported by checkpatch:
> > > > > 
> > > > > WARNING: else is not generally useful after a break or return
> > > > > 
> > > > 
> > > > What checkpatch is telling you here is that the "else" is redundant and
> > > > can be removed. Although your patch suppresses the warning, it makes the
> > > > code messier :(
> > > 
> > > Ah wait, after reading Julia's comment, I realize that the "else" is not
> > > redundant at all. Seems like checkpatch.pl is lying. So ignore what I
> > > said.
> > 
> > Thanks
> > 
> > 
> 
> Could you consider fixing checkpatch instead?

Parsing C is quite hard and checkpatch is never going to do it well.
It might be fun to start this project but it's kind of doomed.  Doomed
projects can be an educational experience as well.

The way to do it might be to add a new in_else_if variable which tracks
if you are in an else if block.  You would look at the indent level and
try the curly braces.  Then if the previous thing was an else_if silence
the warning.

But also it's fine that checkpatch prints the occasional incorrect
warning because it teaches people to be more careful.

Another thing is that when you introduce a bug, you should always
consider if other people have done that before as well.  Perhaps do
a `git log -p --grep "else is not generally useful"` and see if everyone
else did it correctly and if reviewers caught the mistakes.

regards,
dan carpenter