linux-kernel - Re: [PATCH 3/5] checkpatch: add a blacklist

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] [day] [month] [year] [list]

Message-Id: <1254952735.18167.350.camel@desktop>
Date:	Wed, 07 Oct 2009 14:58:55 -0700
From:	Daniel Walker <dwalker@...o99.com>
To:	Krzysztof Halasa <khc@...waw.pl>
Cc:	rostedt@...dmis.org, Andy Whitcroft <apw@...onical.com>,
	Li Zefan <lizf@...fujitsu.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/5] checkpatch: add a blacklist

On Wed, 2009-10-07 at 23:30 +0200, Krzysztof Halasa wrote:
> Daniel Walker <dwalker@...o99.com> writes:
> 
> > Just relax the submission rules so that checkpatch is basically an
> > optional part of the submission process. Adding that you don't actually
> > need to run it, you don't need a good reason not to follow the rules
> > etc.. Or expand on it to fully explain what you think the deal is or
> > should be.
> 
> Oh come on... The SubmittingPatches is a HOWTO-style document for people
> who want to get their patches merged. Obviously a common sense dictates
> you need a good reason to ignore this or that. "Looks better" and
> "I find it easier to work with" are good reasons since this is source
> code, for humans to work with.

The whole point of this black list is to allow people to give good
reasons why they don't follow the tool.. Once in the blacklist those
errors that are getting ignored become formally ignored.. So I haven't
been against people having a good reason not to fix the errors, but you
still need a good reason. I've checked 1000's of patches and Steven's
code is the only one that I've found which uniformly violates the coding
style ..

> BTW, the file says:
> 
> "Check your patches with the patch style checker prior to submission
> (scripts/checkpatch.pl).  The style checker should be viewed as
> a guide not as the final word.  If your code looks better with
>   ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> a violation then its probably best left alone.

I think that exists since the tool can parse incorrectly sometimes and
flag things that it shouldn't be flagging (often times are checkpatch
defects).. For instance a large macro that only creates a function would
not need to be wrapped in a "do { } while(0);" .. So in those cases the
tool can't be the final word, this list (or a list) needs to be the
final word. Usually in those cases there is no need to explain the error
since it's clear what's happening in the code..

> The checker reports at three levels:
>  - ERROR: things that are very likely to be wrong
>                                ^^^^^^

Well, "very likely" is pretty strong wording for a this kind of tool ..

>  - WARNING: things requiring careful review
>  - CHECK: things requiring thought"
> 
> Only "very likely".
> 
> WRT tabs vs spaces, I wonder if using only spaces would be a better
> idea. Theoretically using tabs for syntactic indentation only is better,
> but the tools (editors) aren't up to the task yet.

You can usually change your editor to conform .. I know emacs usually
does spaces for tabs, but I'm sure you can change it's config to use
real tabs .. I don't use emacs tho.

Daniel

--
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/