From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755203AbcH1BHL (ORCPT ); Sat, 27 Aug 2016 21:07:11 -0400 Received: from omzsmtpe01.verizonbusiness.com ([199.249.25.210]:51762 "EHLO omzsmtpe01.verizonbusiness.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754353AbcH1BHK (ORCPT ); Sat, 27 Aug 2016 21:07:10 -0400 X-IronPort-Anti-Spam-Filtered: false From: "Levin, Alexander" Cc: Sasha Levin , "Levin, Alexander" , Greg KH , LKML , "ksummit-discuss@lists.linuxfoundation.org" X-IronPort-AV: E=Sophos;i="5.28,588,1464652800"; d="scan'208";a="189151611" To: Joe Perches Date: Sat, 27 Aug 2016 21:06:13 -0400 Subject: Re: checkkpatch (in)sanity ? Thread-Topic: checkkpatch (in)sanity ? Thread-Index: AdIAyF8RuIojF5/6QbuaUNmJcHkU/A== Message-ID: <20160828005636.GB19088@sasha-lappy> References: <1472330452.26978.23.camel@perches.com> In-Reply-To: <1472330452.26978.23.camel@perches.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Mutt/1.5.24 (2015-08-30) acceptlanguage: en-US Content-Type: text/plain; charset="iso-8859-1" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id u7S17Hba016647 On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote: > On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote: > > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote: > >>    - Making checkpatch check for (some) of the stable kernel rules > >>    (and possibly recommend adding the stable@ tag in certain cases?). > >>      - Depends on: making checkpatch sane again.>This sounds interesting.  What do you mean by "sane"? > > Sasha, can you expand your thoughts here please? Sure. I have 2.5 concerns about the state of checkpatch: On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote: > Most all of the trivial spacing stuff can easily be > ignored either by a human determining what's important > or by using command line options like --ignore=spacing 1. This is the wrong default. By default checkpatch shouldn't be showing trivial issues that encourage folks to try and work around them and as a result produce worse code. Look at the 80 character limit warning for example, what good does it do? It encourages people to do even stupider things to work around it and results in a bunch of "fix checkpatch warning" that touch existing code just to make the result harder to read and make 'git blame' harder to work with. By default you should only get the most critical warnings we have in the kernel like missing S-O-B or corrupt patch. 2. A "who wrote these rules?": there seems to be a disconnect between the rules checkpatch is trying to enforce and the accepted coding style enforced by maintainers. Do a git-format-patch on all of the commits Linus authored in the past year or two and see how many of them fail checkpatch (or do the same for any of the commits that passed through and were accepted by the top maintainers), according to checkpatch we need to make those guys stop touching the kernel. 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of perl code that a fair amount of people don't know how to deal with. In 4.8 it's 6142 lines, making it the 124th largest source file in the kernel, well within the top 1% of source files in the kernel. This combination of size/language pushes people away from being involved in what is supposed to be a central tool and gives them a reason to never use it again after they see results they don't agree with (rather than fixing it). -- Thanks, Sasha