Message-ID: <20160922112407.47da9393@endymion>
Date:   Thu, 22 Sep 2016 11:24:07 +0200
From:   Jean Delvare <jdelvare@...e.de>
To:     Joe Perches <joe@...ches.com>
Cc:     Julia Lawall <julia.lawall@...6.fr>,
        Al Viro <viro@...IV.linux.org.uk>,
        Ilya Dryomov <idryomov@...il.com>,
        Andy Whitcroft <apw@...onical.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Jonathan Corbet <corbet@....net>,
        Ceph Development <ceph-devel@...r.kernel.org>,
        Alex Elder <elder@...nel.org>, Sage Weil <sage@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-janitors@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-doc@...r.kernel.org
Subject: Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

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.

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.

-- 
Jean Delvare
SUSE L3 Support

Powered by blists - more mailing lists