Re: [PATCH] staging: nvec: use x instead of x != NULL

[Julia Lawall]

On Wed, 26 Jun 2024, Philipp Hortmann wrote:

> On 6/26/24 06:48, Julia Lawall wrote:
> >
> >
> > On Wed, 26 Jun 2024, Philipp Hortmann wrote:
> >
> > > On 6/25/24 22:56, Tom Mounet wrote:
> > > > Comply with coding rules defined in checkpatch
> > > >
> > > > Signed-off-by: Tom Mounet <tommounet@xxxxxxxxx>
> > > > ---
> > > >    drivers/staging/nvec/nvec.c | 4 ++--
> > > >    1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> > > > index e5ca78e57..814eb121c 100644
> > > > --- a/drivers/staging/nvec/nvec.c
> > > > +++ b/drivers/staging/nvec/nvec.c
> > > > @@ -300,7 +300,7 @@ int nvec_write_sync(struct nvec_chip *nvec,
> > > >    {
> > > >    	mutex_lock(&nvec->sync_write_mutex);
> > > >    -	if (msg != NULL)
> > > > +	if (msg)
> > > >    		*msg = NULL;
> > > >      	nvec->sync_write_pending = (data[1] << 8) + data[0];
> > > > @@ -322,7 +322,7 @@ int nvec_write_sync(struct nvec_chip *nvec,
> > > >      	dev_dbg(nvec->dev, "nvec_sync_write: pong!\n");
> > > >    -	if (msg != NULL)
> > > > +	if (msg)
> > > >    		*msg = nvec->last_sync_msg;
> > > >    	else
> > > >    		nvec_msg_free(nvec, nvec->last_sync_msg);
> > >
> > >
> > > Hi Tom,
> > >
> > > what you change in this patch is fine. But the Description is not so
> > > lucky.
> > > Reason is that checkpatch is not defining the coding style. Not at all.
> > > Sometimes checkpatch is even wrong. The description I like would be:
> > >
> > > Use x instead of x != NULL to shorten code.
> > >
> > > or
> > >
> > > Use x instead of x != NULL to improve readability.
> > >
> > > If you send in a second version of this patch please use a change history.
> > > Description from Dan under:
> > > https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-patch/
> >
> > How about adding "Issue identified by checkpatch"?  Checkpatch helped find
> > the problem, so it would be nice to acknowledge that.
> >
> > julia
> >
>
> Hi Julia,
>
> The following lines sound very authoritative. It is only my opinion and can be
> wrong.
>
> I think checkpatch is valued a lot because every patch send in is checked by
> checkpatch. checkpatch can be mentioned in the description. But the developer
> cannot hide at all behind a checkpatch warning/error message. The developer
> must take full responsibility for the patch. The developer needs to use common
> sense.
>
> Please have a look at this email from Greg:
> https://lore.kernel.org/linux-staging/2024062443-udder-spotted-cc0d@gregkh/T/#m280ebb2be94e434234f405e722fc35dc6d1db710
>
> I think that Greg once wrote that he does not care about the tool that found
> the issue. He much more cares about if the change makes sense. The "Why" in
> the description is most important for him. And the why cannot be because
> checkpatch or any other tool told the developer so.

Of course.  I was only suggesting to acknowledge the help of checkpatch in
addition to one of the sentences that you proposed.

julia

>
> Thanks for your support.
>
> Bye Philipp
>
>
>
>
>
>
>
>