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 -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
