linux-kernel - Re: [PATCH] scripts/checkpatch.pl: Add warning about leading contination tests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20091206195345.0c7d188a@hyperion.delvare>
Date:	Sun, 6 Dec 2009 19:53:45 +0100
From:	Jean Delvare <khali@...ux-fr.org>
To:	Joe Perches <joe@...ches.com>
Cc:	Eric Dumazet <eric.dumazet@...il.com>,
	Andy Whitcroft <apw@...onical.com>,
	David Miller <davem@...emloft.net>,
	LKML <linux-kernel@...r.kernel.org>,
	William Allen Simpson <william.allen.simpson@...il.com>
Subject: Re: [PATCH] scripts/checkpatch.pl: Add warning about leading 
 contination tests

On Sun, 06 Dec 2009 09:46:16 -0800, Joe Perches wrote:
> On Sun, 2009-12-06 at 13:13 +0100, Jean Delvare wrote:
> > Not fine with me. Placing the binary logic operator at the beginning
> > of a line can be a deliberate choice, either to make complex binary
> > expressions more readable, or to avoid long lines. I don't see much
> > point in banning this style, which BTW is used over 8000 times in the
> > current kernel tree.
> 
> Anyone that thinks that checkpatch is the
> last word on linux coding style and all of
> its pronouncements must be followed all the
> time is simply wrong.
> 
> It's not a ban.  It's neither a command nor
> an edict.  It's a warning.  It's a notice
> that leading logical continuations are not
> the preferred style and it can be ignored
> at will.

I don't disagree with that. However, adding more and more checks in
checkpatch.pl has its downsides. For example, I want to be able to tell
people who submit patches to me: "run checkpatch.pl on your patch and
solve every problem it reports before sending it to me again". If I
must instead tell them: "run checkpatch.pl on your patch and fix what
you want" then they might as well not fix anything, because they will
not know which warnings _I_ find relevant and which I don't. Then the
checkpatch.pl script becomes useless for that use case.

More generally, if checkpatch.pl starts reporting too many warnings
which people consider false positives, then developers may simply stop
using it. This especially holds for newcomers. If they check their
patch and they get 10 errors or warnings, they'll fix them. If they get
100 they may just give up. checkpatch.pl is a wonderful tool and I
don't want to lose that.

So if you are going to add checks which are icing on the cake, please
disable them by default and only show them if the user explicitly asks
for it by passing --strict or some such. As a matter of fact, the rule
you are trying to add is not in Documentation/CodingStyle, as Eric
noticed already, so it can't be that important, can it?

> I think it's rather like the long line, >80
> column warning.  There are a whole lot more
> than 8k long lines in kernel source and no
> one is suggesting reformatting all of them
> out of existence.

Lines longer than 8_k_? I hope not ;)

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/