Re: [PATCH] platform/x86: intel-vbtn: Fix code style issues

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4098 bytes --]

On Wed, 25 Jun 2025, Xiang Shen wrote:

> On Tue, Jun 24, 2025 at 01:35:57PM +1000, Ilpo Järvinen wrote:
> > On Sun, 22 Jun 2025, Xiang Shen wrote:
> > > On Fri, Jun 20, 2025 at 12:00:03PM +1000, Hans de Goede wrote:
> > > > On 20-Jun-25 2:38 AM, Xiang Shen wrote:
> > > > > Fix checkpatch code style errors:
> > > > >
> > > > > ERROR: do not use assignment in if condition
> > > > > +	if ((ke = sparse_keymap_entry_from_scancode(priv->buttons_dev,
> > > event))) {
> > > > >
> > > > > ERROR: do not use assignment in if condition
> > > > > +	} else if ((ke =
> > > sparse_keymap_entry_from_scancode(priv->switches_dev, event))) {
> > > > >
> > > > > Signed-off-by: Xiang Shen <turyshen@gmail.com>
> > > >
> > > > Thank you for your patch, but this change really does not make
> > > > the code more readable.
> > > >
> > > > The contrary the suggested changes are making the code harder
> > > > to read, so NACK.
> > > >
> > > > Note checkpatch is just a tool, sometimes there are good reasons
> > > > to deviate from the style checks done by checkpatch.
> > > >
> > > > Next time when submitting a patch to fix checkpatch issues please
> > > > take a look at the resulting code after the patch and only submit
> > > > the patch upstream if it actually is an improvement.
> > > >
> > > > Regards,
> > > >
> > > > Hans
> > > >
> > > Hi Hans,
> > > 
> > > Thanks for the feedback.
> > > 
> > > That's fine if breaking the "rule" is the only way to keep the file
> > > readable.
> > > 
> > > However, there are only three files (x86/sony-laptop.c and
> > > x86/dell/dell_rbu.c) out of 273 files in the whole drivers/platform
> > > folder that have such an error.
> > 
> > Hi,
> > 
> > Please don't call correct code "error" even if checkpatch may label it as
> > such. The goal is NOT and will never be to have zero checkpatch warnings.
> > 
> > The fact that the checkpatch "rule" is broken only a few times does not
> > mean those 3 places have a problem, it just tells it's good rule for the
> > general case. So I won't accept using such numbers as a leverage against
> > the few places just for the sake of silencing checkpatch.
> > 
>
> I just thought there must be a reason that the checkpatch categories findings
> as "ERROR", "WARNING" and "CHECK".

The checkpatch change submitter just picked one of the levels. They're 
humans too. :-)

> Sometimes the number does make sense and means the vast majority
> follow the widely accepted "rule".
>
> Curiously, isn't it the contributor's due diligence to pass checkpatch
> in the first place before sending?

Hey, that code you're changing is not being submitted. We don't want to 
waste our time on "fixing" checkpatch warnings on existing code when the 
"fix" results in worse readability that before.

Also, if you would be submitting a patch and checkpatch suggest you 
to make the patch worse, please don't follow checkpatch's advice even in 
that case!

Checker tools are there to help find potential issues, not rulebooks. This 
is often overlooked by many checker tool focused developers and results in 
low quality patches (there are exceptions too where they developer really 
tries to understand all the related code to see if the change makes sense 
/ improves things or not).

> Should any objection, submit a patch to
> checkpatch itself,
> instead of sneaking into the upstream quietly for the sake of "readability".
>
> > > Perhaps there are other approaches to make them more readable without
> > > breaking the rule.
> > 
> > Perhaps, but I'm not sure the effort spent to find one is worthwhile
> > investment.
> > 
> 
> Indeed, that's precisely why it might be worth sacrificing a bit of
> "readability".

Please stop trying to sneak your subpar change in.

NO, this a hard NO from the acting maintainer. We'll not be accepting 
misguided changes that are to silence ANY checker that should have know 
better (those checkers will never learn enough to be relied 100% so 
please stop spreading such illusion).


-- 
 i.