From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933654AbcIVKnz convert rfc822-to-8bit (ORCPT ); Thu, 22 Sep 2016 06:43:55 -0400 Received: from mga07.intel.com ([134.134.136.100]:40962 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932551AbcIVKnv (ORCPT ); Thu, 22 Sep 2016 06:43:51 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,378,1470726000"; d="scan'208";a="1044117166" From: Jani Nikula To: Jean Delvare , Joe Perches Cc: Julia Lawall , Al Viro , Ilya Dryomov , Andy Whitcroft , Linus Torvalds , Jonathan Corbet , Ceph Development , Alex Elder , Sage Weil , LKML , kernel-janitors@vger.kernel.org, Andrew Morton , linux-doc@vger.kernel.org Subject: Re: "CodingStyle: Clarify and complete chapter 7" in docs-next In-Reply-To: <20160922112407.47da9393@endymion> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20160920001159.GM2356@ZenIV.linux.org.uk> <1474339566.1954.25.camel@perches.com> <1474353123.1954.28.camel@perches.com> <20160922112407.47da9393@endymion> User-Agent: Notmuch/0.22.1+62~g2a7b11b (https://notmuchmail.org) Emacs/24.4.1 (x86_64-pc-linux-gnu) Date: Thu, 22 Sep 2016 13:43:42 +0300 Message-ID: <8760pop63l.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 22 Sep 2016, Jean Delvare wrote: > Hi Joe, > > On Mon, 19 Sep 2016 23:32:03 -0700, Joe Perches wrote: >> On Tue, 2016-09-20 at 07:53 +0200, Julia Lawall wrote: >> > I think it is better to be clear.  CHECK was never really clear to me, >> > especially if you see it in isolation, on a file that doesn't also have >> > ERROR or WARNING.  NITS is a common word in this context, but not FLEAS >> > and GNATS, as far as I know. >> > There could also be a severity level: high medium and low >> >> I agree clarity is good. >> >> The seriousness with which some beginners take these message >> types though is troublesome, > > You need to think in terms of actual use cases. Who uses checkpatch and > why? I think there are 3 groups of users: > * Beginners. They won't run the script by themselves, instead they will > submit a patch which infringes a lot of coding style rules, and the > maintainer will point them to checkpatch and ask for a resubmission > which makes checkpatch happy. Being beginners, they can only rely on > the script itself to only report things which need to be fixed, by > default. > * Experienced developers. Who simply want to make sure they did not > overlook anything before they post their work for review. They have > the knowledge to decide if they want to ignore some of the warnings. > * People with too much spare time, looking for anything they could > "contribute" to the kernel. They will use --subjective and piss off > every maintainer they can find. > > Sadly there's not much we can do about the third category, short of > killing option --subjective altogether. You could make checkpatch have different defaults for patches and files, to encourage better style in new code, but to discourage finding problems in existing code. BR, Jani. > > The default settings should work out of the box for for the first > category. > >> Maybe prefix various different types of style messages. >> >> Something like: >> >> ERROR -> CODE_STYLE_DEFECT >> WARNING -> CODE_STYLE_UNPREFERRED >> CHECK -> CODE_STYLE_NIT > > I don't think you clarify anything with these changes, as they do not > carry the requirement from a submitter's perspective. If you really > want to change the names, I would rather suggest: > > ERROR -> MUST_FIX > WARNING -> SHOULD_FIX > CHECK -> MAY_FIX > > Or explain the categories in plain English, see below. > >> I doubt additional external documentation would help much. > > I agree. If anything needs to be explained, it should be included in > the output of checkpatch directly. For example, if any ERROR was > printed, include the following after the count summary: > > "ERROR means the code is infringing a core coding style rule. This MUST > be fixed before the patch is submitted upstream." > > And if any WARNING was printed, include the following: > > "WARNING means the code is not following the best practice. This SHOULD > be fixed before the patch is submitted upstream, unless the maintainer > agreed otherwise." > > Not sure if we need something for CHECK, as these messages are not > printed by default so I'd assume people who get them know what they > asked for. But apparently these confused Julia so maybe a similar > explanation would be needed for them too. -- Jani Nikula, Intel Open Source Technology Center