"CodingStyle: Clarify and complete chapter 7" in docs-next (was Re: [PATCH 03/47] block-rbd: Adjust the position of a jump label in rbd_header_from_disk())

"CodingStyle: Clarify and complete chapter 7" in docs-next (was Re: [PATCH 03/47] block-rbd: Adjust the position of a jump label in rbd_header_from_disk())

[Ilya Dryomov]

On Mon, Sep 19, 2016 at 11:37 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Ilya,
>
> Sorry for the late answer.
>
> On Tue, 13 Sep 2016 20:31:57 +0200, Ilya Dryomov wrote:
>> Sorry, navigating lkml.org archive is a pain, and I was expecting to
>> see patch.  Your points
>>
>> "The acceptance of an optional single space before labels dates back to
>> at least June 2007, as supported by the very first incarnation of
>> checkpatch.pl. So nothing really new here, except for a preference
>> (my preference, admittedly, but I'm know I'm not alone) being expressed
>> in the coding style document."
>>
>> "Recommendations are not meant to document what people are currently
>> doing but what we think they should be doing."
>>
>> are valid, but note that there is a world of difference between an
>> acceptance and a preference.  The *only* point of whitespace guidelines
>> is to keep the code base consistent.
>
> Consistency is half of the reason, the other half is readability. This
> is why the CodingStyle document has a number of rationales explained.
> This is also why we put whitespace in the first place, while the C
> language doesn't require any ;-)
>
> The sense of my proposal was to address a readability (or usability)
> issue.
>
>> You don't go changing whitespace
>> preferences in such a huge project, not unless you have a *very* good
>> rationale and existing code base is swayed (which it isn't, given the
>> 9/10 ratio).
>
> I did consider the reason to be good enough to warrant a "change",
> actually. Or more exactly from "one space is allowed" to "one space is
> recommended." Which is quite different from changing all the code
> actively. I can understand how you don't like it, but again, this
> "inconsistency" has been accepted for almost a decade now, so I find it
> strange to see so much resistance when someone finally tries to sort it
> out.

Yeah, I guess that's where our disagreement lies - the "so that `diff
-p` does not confuse labels with functions" in the age of git, hg and
others, all of which can be customized to your heart's content is not
a good enough reason to go from "allowed" to "advised".

>
>> >> If I wanted to clarify the
>> >> situation, I'd have gone with "one space indented labels are also
>> >> acceptable" or so.  The example you've re-indented dates back to 2.6.4
>> >> times...
>> >
>> > I can't see how this is relevant.
>>
>> That was a 12 year old example, codifying an existing style used in
>> ~90% cases, serving as a guideline for new contributors.
>
> OK, I get your point now. But the CodingStyle document isn't carved
> into stone. I see 43 changes to that file in recent history (since
> April 2005), some of which are actual changes or clarifications of our
> coding style. This very section of the document was updated in December
> 2014, so not so long ago.
>
> In the end I suppose it boils down to how problematic you consider the
> current situation to be. Apparently you and several other maintainers
> think it's just fine, while me (and a few others apparently) think it
> is not.
>
>> >> git diff also works on regular files, BTW.
>> >
>> > I have no idea what you mean here, sorry.
>>
>> Oh, just that it works outside of git repos too, so you aren't stuck
>> with diffutils if you want to diff two random .c files.
>
> Oh, I had never thought of that. Thanks for the hint :-)
>
>> > (...)
>> > http://marc.info/?l=linux-kernel&m=147325166209844&w=2
>> >
>> > It uses the git diff xfuncname feature you mentioned above. To be
>> > honest I'm surprised it isn't the git default, it seems odd to have so
>> > many diff drivers included in git and not enable them on obvious file
>> > extensions. Oh well.
>>
>> This came up before: http://www.spinics.net/lists/git/msg164216.html,
>> Linus didn't like it.  I suggest you add him to the CC on this patch to
>> see if he changed his mind.
>
> Thanks for the pointer. It is interesting to see many people had been
> bothered by the same problem for many years and even proposed solution
> for it. But also sad to see that nothing happened :-(
>
> Well Linus suggested to improve the default, he was not opposed to the
> change per se I think. But it was 5 years ago and nothing happened
> since then, so I'd rather go with what is available today. Which means
> either one space before labels, or drivers in .gitattributes. Choose
> your poison ;-)

Jon, ping?  My points upthread aside, both CodingStyle and
.gitattributes patches seem to be queued...

Thanks,

                Ilya

"CodingStyle: Clarify and complete chapter 7" in docs-next (was Re: [PATCH 03/47] block-rbd: Adjust

[Ilya Dryomov]

On Mon, Sep 19, 2016 at 11:37 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Ilya,
>
> Sorry for the late answer.
>
> On Tue, 13 Sep 2016 20:31:57 +0200, Ilya Dryomov wrote:
>> Sorry, navigating lkml.org archive is a pain, and I was expecting to
>> see patch.  Your points
>>
>> "The acceptance of an optional single space before labels dates back to
>> at least June 2007, as supported by the very first incarnation of
>> checkpatch.pl. So nothing really new here, except for a preference
>> (my preference, admittedly, but I'm know I'm not alone) being expressed
>> in the coding style document."
>>
>> "Recommendations are not meant to document what people are currently
>> doing but what we think they should be doing."
>>
>> are valid, but note that there is a world of difference between an
>> acceptance and a preference.  The *only* point of whitespace guidelines
>> is to keep the code base consistent.
>
> Consistency is half of the reason, the other half is readability. This
> is why the CodingStyle document has a number of rationales explained.
> This is also why we put whitespace in the first place, while the C
> language doesn't require any ;-)
>
> The sense of my proposal was to address a readability (or usability)
> issue.
>
>> You don't go changing whitespace
>> preferences in such a huge project, not unless you have a *very* good
>> rationale and existing code base is swayed (which it isn't, given the
>> 9/10 ratio).
>
> I did consider the reason to be good enough to warrant a "change",
> actually. Or more exactly from "one space is allowed" to "one space is
> recommended." Which is quite different from changing all the code
> actively. I can understand how you don't like it, but again, this
> "inconsistency" has been accepted for almost a decade now, so I find it
> strange to see so much resistance when someone finally tries to sort it
> out.

Yeah, I guess that's where our disagreement lies - the "so that `diff
-p` does not confuse labels with functions" in the age of git, hg and
others, all of which can be customized to your heart's content is not
a good enough reason to go from "allowed" to "advised".

>
>> >> If I wanted to clarify the
>> >> situation, I'd have gone with "one space indented labels are also
>> >> acceptable" or so.  The example you've re-indented dates back to 2.6.4
>> >> times...
>> >
>> > I can't see how this is relevant.
>>
>> That was a 12 year old example, codifying an existing style used in
>> ~90% cases, serving as a guideline for new contributors.
>
> OK, I get your point now. But the CodingStyle document isn't carved
> into stone. I see 43 changes to that file in recent history (since
> April 2005), some of which are actual changes or clarifications of our
> coding style. This very section of the document was updated in December
> 2014, so not so long ago.
>
> In the end I suppose it boils down to how problematic you consider the
> current situation to be. Apparently you and several other maintainers
> think it's just fine, while me (and a few others apparently) think it
> is not.
>
>> >> git diff also works on regular files, BTW.
>> >
>> > I have no idea what you mean here, sorry.
>>
>> Oh, just that it works outside of git repos too, so you aren't stuck
>> with diffutils if you want to diff two random .c files.
>
> Oh, I had never thought of that. Thanks for the hint :-)
>
>> > (...)
>> > http://marc.info/?l=linux-kernel&m\x147325166209844&w=2
>> >
>> > It uses the git diff xfuncname feature you mentioned above. To be
>> > honest I'm surprised it isn't the git default, it seems odd to have so
>> > many diff drivers included in git and not enable them on obvious file
>> > extensions. Oh well.
>>
>> This came up before: http://www.spinics.net/lists/git/msg164216.html,
>> Linus didn't like it.  I suggest you add him to the CC on this patch to
>> see if he changed his mind.
>
> Thanks for the pointer. It is interesting to see many people had been
> bothered by the same problem for many years and even proposed solution
> for it. But also sad to see that nothing happened :-(
>
> Well Linus suggested to improve the default, he was not opposed to the
> change per se I think. But it was 5 years ago and nothing happened
> since then, so I'd rather go with what is available today. Which means
> either one space before labels, or drivers in .gitattributes. Choose
> your poison ;-)

Jon, ping?  My points upthread aside, both CodingStyle and
.gitattributes patches seem to be queued...

Thanks,

                Ilya

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next (was Re: [PATCH 03/47] block-rbd: Adjust the position of a jump label in rbd_header_from_disk())

[Al Viro]

On Mon, Sep 19, 2016 at 01:53:37PM +0200, Ilya Dryomov wrote:
> > I did consider the reason to be good enough to warrant a "change",
> > actually. Or more exactly from "one space is allowed" to "one space is
> > recommended." Which is quite different from changing all the code
> > actively. I can understand how you don't like it, but again, this
> > "inconsistency" has been accepted for almost a decade now, so I find it
> > strange to see so much resistance when someone finally tries to sort it
> > out.
> 
> Yeah, I guess that's where our disagreement lies - the "so that `diff
> -p` does not confuse labels with functions" in the age of git, hg and
> others, all of which can be customized to your heart's content is not
> a good enough reason to go from "allowed" to "advised".

On the top of CodingStyle we have
"This is a short document describing the preferred coding style for the
linux kernel."

Let's make it s/describing/& (as in "descriptive, not prescriptive")/.
The lack of consensus (in the core kernel areas) is _not_ an invitation
to pick a rule out of one's arse/nose/armpit/textbook/whatnot and stick
it as The One True Style, and references to uniformity are worthless
in such case.

In particular, "whitespace before labels" kind of patches anywhere in
VFS will be simply rejected.

IMO what we need is to go through all rules in CodingStyle and if for
some rule there is no overwhelming majority in the core kernel, well,
the list has grown way too large and could use massive trimming.

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next (was Re: [PATCH 03/47] block-rbd: Adj

[Al Viro]

On Mon, Sep 19, 2016 at 01:53:37PM +0200, Ilya Dryomov wrote:
> > I did consider the reason to be good enough to warrant a "change",
> > actually. Or more exactly from "one space is allowed" to "one space is
> > recommended." Which is quite different from changing all the code
> > actively. I can understand how you don't like it, but again, this
> > "inconsistency" has been accepted for almost a decade now, so I find it
> > strange to see so much resistance when someone finally tries to sort it
> > out.
> 
> Yeah, I guess that's where our disagreement lies - the "so that `diff
> -p` does not confuse labels with functions" in the age of git, hg and
> others, all of which can be customized to your heart's content is not
> a good enough reason to go from "allowed" to "advised".

On the top of CodingStyle we have
"This is a short document describing the preferred coding style for the
linux kernel."

Let's make it s/describing/& (as in "descriptive, not prescriptive")/.
The lack of consensus (in the core kernel areas) is _not_ an invitation
to pick a rule out of one's arse/nose/armpit/textbook/whatnot and stick
it as The One True Style, and references to uniformity are worthless
in such case.

In particular, "whitespace before labels" kind of patches anywhere in
VFS will be simply rejected.

IMO what we need is to go through all rules in CodingStyle and if for
some rule there is no overwhelming majority in the core kernel, well,
the list has grown way too large and could use massive trimming.

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next (was Re: [PATCH 03/47] block-rbd: Adjust the position of a jump label in rbd_header_from_disk())

[Joe Perches]

On Tue, 2016-09-20 at 01:11 +0100, Al Viro wrote:
> IMO what we need is to go through all rules in CodingStyle and if for
> some rule there is no overwhelming majority in the core kernel, well,
> the list has grown way too large and could use massive trimming.

I'm in complete agreement.

I also think that checkpatch's ERROR/WARNING/CHECK message naming is
far too severe and injunctive and could use a renaming to something
more silly, bug related and less commanding like FLEAS/GNATS/NITS.

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next (was Re: [PATCH 03/47] block-rbd: Adj

[Joe Perches]

On Tue, 2016-09-20 at 01:11 +0100, Al Viro wrote:
> IMO what we need is to go through all rules in CodingStyle and if for
> some rule there is no overwhelming majority in the core kernel, well,
> the list has grown way too large and could use massive trimming.

I'm in complete agreement.

I also think that checkpatch's ERROR/WARNING/CHECK message naming is
far too severe and injunctive and could use a renaming to something
more silly, bug related and less commanding like FLEAS/GNATS/NITS.

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next (was Re: [PATCH 03/47] block-rbd: Adjust the position of a jump label in rbd_header_from_disk())

[Julia Lawall]

On Mon, 19 Sep 2016, Joe Perches wrote:

> On Tue, 2016-09-20 at 01:11 +0100, Al Viro wrote:
> > IMO what we need is to go through all rules in CodingStyle and if for
> > some rule there is no overwhelming majority in the core kernel, well,
> > the list has grown way too large and could use massive trimming.
>
> I'm in complete agreement.
>
> I also think that checkpatch's ERROR/WARNING/CHECK message naming is
> far too severe and injunctive and could use a renaming to something
> more silly, bug related and less commanding like FLEAS/GNATS/NITS.

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.

julia

>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next (was Re: [PATCH 03/47] block-rbd: Adj

[Julia Lawall]

On Mon, 19 Sep 2016, Joe Perches wrote:

> On Tue, 2016-09-20 at 01:11 +0100, Al Viro wrote:
> > IMO what we need is to go through all rules in CodingStyle and if for
> > some rule there is no overwhelming majority in the core kernel, well,
> > the list has grown way too large and could use massive trimming.
>
> I'm in complete agreement.
>
> I also think that checkpatch's ERROR/WARNING/CHECK message naming is
> far too severe and injunctive and could use a renaming to something
> more silly, bug related and less commanding like FLEAS/GNATS/NITS.

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.

julia

>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next (was Re: [PATCH 03/47] block-rbd: Adjust the position of a jump label in rbd_header_from_disk())

[Joe Perches]

On Tue, 2016-09-20 at 07:53 +0200, Julia Lawall wrote:
> On Mon, 19 Sep 2016, Joe Perches wrote:
> > On Tue, 2016-09-20 at 01:11 +0100, Al Viro wrote:
> > > IMO what we need is to go through all rules in CodingStyle and if for
> > > some rule there is no overwhelming majority in the core kernel, well,
> > > the list has grown way too large and could use massive trimming.
> >
> > I'm in complete agreement.
> >
> > I also think that checkpatch's ERROR/WARNING/CHECK message naming is
> > far too severe and injunctive and could use a renaming to something
> > more silly, bug related and less commanding like FLEAS/GNATS/NITS.
> 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,

Maybe prefix various different types of style messages.

Something like:

ERROR	-> CODE_STYLE_DEFECT
WARNING -> CODE_STYLE_UNPREFERRED
CHECK	-> CODE_STYLE_NIT

I doubt additional external documentation would help much.

Some checkpatch bleats really are errors though.

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next (was Re: [PATCH 03/47] block-rbd: Adj

[Joe Perches]

On Tue, 2016-09-20 at 07:53 +0200, Julia Lawall wrote:
> On Mon, 19 Sep 2016, Joe Perches wrote:
> > On Tue, 2016-09-20 at 01:11 +0100, Al Viro wrote:
> > > IMO what we need is to go through all rules in CodingStyle and if for
> > > some rule there is no overwhelming majority in the core kernel, well,
> > > the list has grown way too large and could use massive trimming.
> >
> > I'm in complete agreement.
> >
> > I also think that checkpatch's ERROR/WARNING/CHECK message naming is
> > far too severe and injunctive and could use a renaming to something
> > more silly, bug related and less commanding like FLEAS/GNATS/NITS.
> 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,

Maybe prefix various different types of style messages.

Something like:

ERROR	-> CODE_STYLE_DEFECT
WARNING -> CODE_STYLE_UNPREFERRED
CHECK	-> CODE_STYLE_NIT

I doubt additional external documentation would help much.

Some checkpatch bleats really are errors though.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next (was Re: [PATCH 03/47] block-rbd: Adjust the position of a jump label in rbd_header_from_disk())

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1637 bytes --]



On Mon, 19 Sep 2016, Joe Perches wrote:

> On Tue, 2016-09-20 at 07:53 +0200, Julia Lawall wrote:
> > On Mon, 19 Sep 2016, Joe Perches wrote:
> > > On Tue, 2016-09-20 at 01:11 +0100, Al Viro wrote:
> > > > IMO what we need is to go through all rules in CodingStyle and if for
> > > > some rule there is no overwhelming majority in the core kernel, well,
> > > > the list has grown way too large and could use massive trimming.
> > >
> > > I'm in complete agreement.
> > >
> > > I also think that checkpatch's ERROR/WARNING/CHECK message naming is
> > > far too severe and injunctive and could use a renaming to something
> > > more silly, bug related and less commanding like FLEAS/GNATS/NITS.
> > 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,

It's not necessarily the case that changing the error type will change the
behavior of the persons in question.

> Maybe prefix various different types of style messages.
>
> Something like:
>
> ERROR	-> CODE_STYLE_DEFECT
> WARNING -> CODE_STYLE_UNPREFERRED
> CHECK	-> CODE_STYLE_NIT
>
> I doubt additional external documentation would help much.
>
> Some checkpatch bleats really are errors though.

Maybe just downgrade more things?

Perhaps SUGGESTION would be more clear than CHECK?

julia

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next (was Re: [PATCH 03/47] block-rbd: Adj

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1683 bytes --]



On Mon, 19 Sep 2016, Joe Perches wrote:

> On Tue, 2016-09-20 at 07:53 +0200, Julia Lawall wrote:
> > On Mon, 19 Sep 2016, Joe Perches wrote:
> > > On Tue, 2016-09-20 at 01:11 +0100, Al Viro wrote:
> > > > IMO what we need is to go through all rules in CodingStyle and if for
> > > > some rule there is no overwhelming majority in the core kernel, well,
> > > > the list has grown way too large and could use massive trimming.
> > >
> > > I'm in complete agreement.
> > >
> > > I also think that checkpatch's ERROR/WARNING/CHECK message naming is
> > > far too severe and injunctive and could use a renaming to something
> > > more silly, bug related and less commanding like FLEAS/GNATS/NITS.
> > 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,

It's not necessarily the case that changing the error type will change the
behavior of the persons in question.

> Maybe prefix various different types of style messages.
>
> Something like:
>
> ERROR	-> CODE_STYLE_DEFECT
> WARNING -> CODE_STYLE_UNPREFERRED
> CHECK	-> CODE_STYLE_NIT
>
> I doubt additional external documentation would help much.
>
> Some checkpatch bleats really are errors though.

Maybe just downgrade more things?

Perhaps SUGGESTION would be more clear than CHECK?

julia

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jean Delvare]

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

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jean Delvare]

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

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Joe Perches]

On Thu, 2016-09-22 at 11:24 +0200, Jean Delvare wrote:
[]
> > 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.

I think you overlook the category of a beginner submitting
"my first kernel patch" which is a "coding style" defect of
some type.  The Eudyptula and Outreachy programs seem to
encourage these sorts of patches.

This is where "scripts/checkpatch.pl -f <file>" is most used.

I believe adding the --force option might be useful to
restrict cleanup-style-only patches outside of staging.

There's nothing wrong with cleanup style patches, it can be
good introduction to compiler/config tool & kernel setup.
 
> I would rather suggest:
> 
> ERROR -> MUST_FIX
> WARNING -> SHOULD_FIX
> CHECK -> MAY_FIX

MUST is much stronger language than I would prefer.

There are still about a quarter million ERRORs just for
spacing issues in the kernel tree.

Here are the top 10 ERROR checkpatch messages treewide as of
a few days ago,

$ grep ERROR checkpatch.short_sorted_20160917
 268308  ERROR:SPACING
  37340  ERROR:CODE_INDENT
  27678  ERROR:TRAILING_WHITESPACE
  21024  ERROR:COMPLEX_MACRO
  14048  ERROR:POINTER_LOCATION
  12207  ERROR:TRAILING_STATEMENTS
  11079  ERROR:OPEN_BRACE
   6802  ERROR:ASSIGN_IN_IF
   3940  ERROR:RETURN_PARENTHESES
   2322  ERROR:NON_OCTAL_PERMISSIONS

Maybe there could be some better classifications of the various
messages.

But there are about two million checkpatch messages overall in
the kernel tree.

That's a lot.

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Joe Perches]

On Thu, 2016-09-22 at 11:24 +0200, Jean Delvare wrote:
[]
> > 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.

I think you overlook the category of a beginner submitting
"my first kernel patch" which is a "coding style" defect of
some type.  The Eudyptula and Outreachy programs seem to
encourage these sorts of patches.

This is where "scripts/checkpatch.pl -f <file>" is most used.

I believe adding the --force option might be useful to
restrict cleanup-style-only patches outside of staging.

There's nothing wrong with cleanup style patches, it can be
good introduction to compiler/config tool & kernel setup.
 
> I would rather suggest:
> 
> ERROR -> MUST_FIX
> WARNING -> SHOULD_FIX
> CHECK -> MAY_FIX

MUST is much stronger language than I would prefer.

There are still about a quarter million ERRORs just for
spacing issues in the kernel tree.

Here are the top 10 ERROR checkpatch messages treewide as of
a few days ago,

$ grep ERROR checkpatch.short_sorted_20160917
 268308  ERROR:SPACING
  37340  ERROR:CODE_INDENT
  27678  ERROR:TRAILING_WHITESPACE
  21024  ERROR:COMPLEX_MACRO
  14048  ERROR:POINTER_LOCATION
  12207  ERROR:TRAILING_STATEMENTS
  11079  ERROR:OPEN_BRACE
   6802  ERROR:ASSIGN_IN_IF
   3940  ERROR:RETURN_PARENTHESES
   2322  ERROR:NON_OCTAL_PERMISSIONS

Maybe there could be some better classifications of the various
messages.

But there are about two million checkpatch messages overall in
the kernel tree.

That's a lot.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jean Delvare]

On Thu, 22 Sep 2016 03:42:10 -0700, Joe Perches wrote:
> On Thu, 2016-09-22 at 11:24 +0200, Jean Delvare wrote:
> > I would rather suggest:
> > 
> > ERROR -> MUST_FIX
> > WARNING -> SHOULD_FIX
> > CHECK -> MAY_FIX
> 
> MUST is much stronger language than I would prefer.

That's what error means, really. When your compiler fails with an
error, you have no choice but to fix your code. Warnings on the other
hand may be ignored sometimes.

> There are still about a quarter million ERRORs just for
> spacing issues in the kernel tree.
> 
> Here are the top 10 ERROR checkpatch messages treewide as of
> a few days ago,
> 
> $ grep ERROR checkpatch.short_sorted_20160917
>  268308  ERROR:SPACING
>   37340  ERROR:CODE_INDENT
>   27678  ERROR:TRAILING_WHITESPACE
>   21024  ERROR:COMPLEX_MACRO
>   14048  ERROR:POINTER_LOCATION
>   12207  ERROR:TRAILING_STATEMENTS
>   11079  ERROR:OPEN_BRACE
>    6802  ERROR:ASSIGN_IN_IF
>    3940  ERROR:RETURN_PARENTHESES
>    2322  ERROR:NON_OCTAL_PERMISSIONS
> 
> Maybe there could be some better classifications of the various
> messages.
> 
> But there are about two million checkpatch messages overall in
> the kernel tree.
> 
> That's a lot.

Sure. But I'm afraid you keep changing topics and I have no idea where
you are going. We started with "should there be a space before jump
labels", then out of nowhere we were discussing the wording of the
output of checkpatch (how is that related?) and now you pull statistics
out of your hat, like these numbers imply anything.

checkpatch was called checkPATCH for a reason. It's main intent was to
prevent NEW (coding-style mostly) errors from creeping into the kernel.
The fact that old code does now always follow these recommendations is
unfortunate but that doesn't make checkpatch wrong or bad.

ERROR means that the new code isn't allowed to do that. Period.

-- 
Jean Delvare
SUSE L3 Support

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jean Delvare]

On Thu, 22 Sep 2016 03:42:10 -0700, Joe Perches wrote:
> On Thu, 2016-09-22 at 11:24 +0200, Jean Delvare wrote:
> > I would rather suggest:
> > 
> > ERROR -> MUST_FIX
> > WARNING -> SHOULD_FIX
> > CHECK -> MAY_FIX
> 
> MUST is much stronger language than I would prefer.

That's what error means, really. When your compiler fails with an
error, you have no choice but to fix your code. Warnings on the other
hand may be ignored sometimes.

> There are still about a quarter million ERRORs just for
> spacing issues in the kernel tree.
> 
> Here are the top 10 ERROR checkpatch messages treewide as of
> a few days ago,
> 
> $ grep ERROR checkpatch.short_sorted_20160917
>  268308  ERROR:SPACING
>   37340  ERROR:CODE_INDENT
>   27678  ERROR:TRAILING_WHITESPACE
>   21024  ERROR:COMPLEX_MACRO
>   14048  ERROR:POINTER_LOCATION
>   12207  ERROR:TRAILING_STATEMENTS
>   11079  ERROR:OPEN_BRACE
>    6802  ERROR:ASSIGN_IN_IF
>    3940  ERROR:RETURN_PARENTHESES
>    2322  ERROR:NON_OCTAL_PERMISSIONS
> 
> Maybe there could be some better classifications of the various
> messages.
> 
> But there are about two million checkpatch messages overall in
> the kernel tree.
> 
> That's a lot.

Sure. But I'm afraid you keep changing topics and I have no idea where
you are going. We started with "should there be a space before jump
labels", then out of nowhere we were discussing the wording of the
output of checkpatch (how is that related?) and now you pull statistics
out of your hat, like these numbers imply anything.

checkpatch was called checkPATCH for a reason. It's main intent was to
prevent NEW (coding-style mostly) errors from creeping into the kernel.
The fact that old code does now always follow these recommendations is
unfortunate but that doesn't make checkpatch wrong or bad.

ERROR means that the new code isn't allowed to do that. Period.

-- 
Jean Delvare
SUSE L3 Support

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Al Viro]

On Thu, Sep 22, 2016 at 01:57:58PM +0200, Jean Delvare wrote:
> > 
> > MUST is much stronger language than I would prefer.
> 
> That's what error means, really. When your compiler fails with an
> error, you have no choice but to fix your code. Warnings on the other
> hand may be ignored sometimes.

And they are errors, because of...?

> Sure. But I'm afraid you keep changing topics and I have no idea where
> you are going. We started with "should there be a space before jump
> labels", then out of nowhere we were discussing the wording of the
> output of checkpatch (how is that related?) and now you pull statistics
> out of your hat, like these numbers imply anything.
> 
> checkpatch was called checkPATCH for a reason. It's main intent was to
> prevent NEW (coding-style mostly) errors from creeping into the kernel.
> The fact that old code does now always follow these recommendations is
> unfortunate but that doesn't make checkpatch wrong or bad.
> 
> ERROR means that the new code isn't allowed to do that. Period.

The main intent of checkpatch these days appears to be providing an easy
way of thoughtless inflation of commit counts, everything else be damned.
Make-work, in other words.

The _only_ criterion for adding new checks should be a strong consensus in
the core kernel.  IOW, it should be descriptive, not prescriptive.  "Some
people do it this way, some - that" is not a valid reason for "let's make it
uniform; that way is just better, so from now on it's a new requirement".
Especially when the rationale behind the choice has all the intellectual
rigour of feng shui.  Some of these checks are common-sense, some are
absolutely arbitrary, there are far too many of them and elevating them
to the level of compiler errors like you seem to do is rather dishonest.

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Al Viro]

On Thu, Sep 22, 2016 at 01:57:58PM +0200, Jean Delvare wrote:
> > 
> > MUST is much stronger language than I would prefer.
> 
> That's what error means, really. When your compiler fails with an
> error, you have no choice but to fix your code. Warnings on the other
> hand may be ignored sometimes.

And they are errors, because of...?

> Sure. But I'm afraid you keep changing topics and I have no idea where
> you are going. We started with "should there be a space before jump
> labels", then out of nowhere we were discussing the wording of the
> output of checkpatch (how is that related?) and now you pull statistics
> out of your hat, like these numbers imply anything.
> 
> checkpatch was called checkPATCH for a reason. It's main intent was to
> prevent NEW (coding-style mostly) errors from creeping into the kernel.
> The fact that old code does now always follow these recommendations is
> unfortunate but that doesn't make checkpatch wrong or bad.
> 
> ERROR means that the new code isn't allowed to do that. Period.

The main intent of checkpatch these days appears to be providing an easy
way of thoughtless inflation of commit counts, everything else be damned.
Make-work, in other words.

The _only_ criterion for adding new checks should be a strong consensus in
the core kernel.  IOW, it should be descriptive, not prescriptive.  "Some
people do it this way, some - that" is not a valid reason for "let's make it
uniform; that way is just better, so from now on it's a new requirement".
Especially when the rationale behind the choice has all the intellectual
rigour of feng shui.  Some of these checks are common-sense, some are
absolutely arbitrary, there are far too many of them and elevating them
to the level of compiler errors like you seem to do is rather dishonest.

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jean Delvare]

On Thu, 22 Sep 2016 14:11:03 +0100, Al Viro wrote:
> On Thu, Sep 22, 2016 at 01:57:58PM +0200, Jean Delvare wrote:
> > > 
> > > MUST is much stronger language than I would prefer.
> > 
> > That's what error means, really. When your compiler fails with an
> > error, you have no choice but to fix your code. Warnings on the other
> > hand may be ignored sometimes.
> 
> And they are errors, because of...?

Because we decided they were important. Of course there is some
subjectivity to this decision, much like all gcc warnings could be
errors and some errors could be warnings. Which is why gcc allows
enabling/disabling individually.

I assume the current set of errors vs. warnings is the result of some
consensus amongst kernel developers. It can certainly be adjusted if
the consensus changes for whatever reason.

> > Sure. But I'm afraid you keep changing topics and I have no idea where
> > you are going. We started with "should there be a space before jump
> > labels", then out of nowhere we were discussing the wording of the
> > output of checkpatch (how is that related?) and now you pull statistics
> > out of your hat, like these numbers imply anything.
> > 
> > checkpatch was called checkPATCH for a reason. It's main intent was to
> > prevent NEW (coding-style mostly) errors from creeping into the kernel.
> > The fact that old code does now always follow these recommendations is
> > unfortunate but that doesn't make checkpatch wrong or bad.
> > 
> > ERROR means that the new code isn't allowed to do that. Period.
> 
> The main intent of checkpatch these days appears to be providing an easy
> way of thoughtless inflation of commit counts, everything else be damned.
> Make-work, in other words.

Yes, I've noticed the trend too :-( But that's a problem with the
people using the tool, mostly, not with the tool itself.

> The _only_ criterion for adding new checks should be a strong consensus in
> the core kernel.  IOW, it should be descriptive, not prescriptive.  "Some
> people do it this way, some - that" is not a valid reason for "let's make it
> uniform; that way is just better, so from now on it's a new requirement".
> Especially when the rationale behind the choice has all the intellectual
> rigour of feng shui.  Some of these checks are common-sense, some are
> absolutely arbitrary, there are far too many of them (...)

That I agree with, I've complained about the same before. But that
doesn't mean checkpatch is not a wonderfully useful tool. It really is,
if used properly.

> and elevating them
> to the level of compiler errors like you seem to do is rather dishonest.

I see. Well clearly this discussion isn't going anywhere so I'll go
back working on things that really matter. See you.

-- 
Jean Delvare
SUSE L3 Support

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jean Delvare]

On Thu, 22 Sep 2016 14:11:03 +0100, Al Viro wrote:
> On Thu, Sep 22, 2016 at 01:57:58PM +0200, Jean Delvare wrote:
> > > 
> > > MUST is much stronger language than I would prefer.
> > 
> > That's what error means, really. When your compiler fails with an
> > error, you have no choice but to fix your code. Warnings on the other
> > hand may be ignored sometimes.
> 
> And they are errors, because of...?

Because we decided they were important. Of course there is some
subjectivity to this decision, much like all gcc warnings could be
errors and some errors could be warnings. Which is why gcc allows
enabling/disabling individually.

I assume the current set of errors vs. warnings is the result of some
consensus amongst kernel developers. It can certainly be adjusted if
the consensus changes for whatever reason.

> > Sure. But I'm afraid you keep changing topics and I have no idea where
> > you are going. We started with "should there be a space before jump
> > labels", then out of nowhere we were discussing the wording of the
> > output of checkpatch (how is that related?) and now you pull statistics
> > out of your hat, like these numbers imply anything.
> > 
> > checkpatch was called checkPATCH for a reason. It's main intent was to
> > prevent NEW (coding-style mostly) errors from creeping into the kernel.
> > The fact that old code does now always follow these recommendations is
> > unfortunate but that doesn't make checkpatch wrong or bad.
> > 
> > ERROR means that the new code isn't allowed to do that. Period.
> 
> The main intent of checkpatch these days appears to be providing an easy
> way of thoughtless inflation of commit counts, everything else be damned.
> Make-work, in other words.

Yes, I've noticed the trend too :-( But that's a problem with the
people using the tool, mostly, not with the tool itself.

> The _only_ criterion for adding new checks should be a strong consensus in
> the core kernel.  IOW, it should be descriptive, not prescriptive.  "Some
> people do it this way, some - that" is not a valid reason for "let's make it
> uniform; that way is just better, so from now on it's a new requirement".
> Especially when the rationale behind the choice has all the intellectual
> rigour of feng shui.  Some of these checks are common-sense, some are
> absolutely arbitrary, there are far too many of them (...)

That I agree with, I've complained about the same before. But that
doesn't mean checkpatch is not a wonderfully useful tool. It really is,
if used properly.

> and elevating them
> to the level of compiler errors like you seem to do is rather dishonest.

I see. Well clearly this discussion isn't going anywhere so I'll go
back working on things that really matter. See you.

-- 
Jean Delvare
SUSE L3 Support

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Julia Lawall]

> > The main intent of checkpatch these days appears to be providing an easy
> > way of thoughtless inflation of commit counts, everything else be damned.
> > Make-work, in other words.
>
> Yes, I've noticed the trend too :-( But that's a problem with the
> people using the tool, mostly, not with the tool itself.

With respect to at least one person in this category, I don't think that
their main original inspiration was checkpatch in the first place.  More
random information collected from developer comments, that person's own
notion of what good code is, etc.  So there may be limits to what one can
do to checkpatch that will have an impact on this issue.

Furthermore, probably people who use checkpatch in the right way, ie to
check their patches, don't actually mention their use of checkpatch.  They
just fix up their commits and move on.

julia

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Julia Lawall]

> > The main intent of checkpatch these days appears to be providing an easy
> > way of thoughtless inflation of commit counts, everything else be damned.
> > Make-work, in other words.
>
> Yes, I've noticed the trend too :-( But that's a problem with the
> people using the tool, mostly, not with the tool itself.

With respect to at least one person in this category, I don't think that
their main original inspiration was checkpatch in the first place.  More
random information collected from developer comments, that person's own
notion of what good code is, etc.  So there may be limits to what one can
do to checkpatch that will have an impact on this issue.

Furthermore, probably people who use checkpatch in the right way, ie to
check their patches, don't actually mention their use of checkpatch.  They
just fix up their commits and move on.

julia

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Joe Perches]

On Thu, 2016-09-22 at 14:11 +0100, Al Viro wrote:
> The main intent of checkpatch these days appears to be providing an easy
> way of thoughtless inflation of commit counts, everything else be damned.

You've made this statement several times over many years.
I don't believe it's true.
I doubt anyone is getting paid per commit.
Who really cares enough to game some stupid little metric?

If it's really a big deal, name some names.  Otherwise please
stop with this dubious argument/point.

> Some of these checks are common-sense, some are
> absolutely arbitrary,

Essentially all of the checkpatch rules are arbitrary.
The compiler doesn't care one way or another.

All the checks exist just to make code appear more consistent.

The conceit being that more consistent code is easier for humans
to read and spot potential defects and possible reuse patterns.

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Joe Perches]

On Thu, 2016-09-22 at 14:11 +0100, Al Viro wrote:
> The main intent of checkpatch these days appears to be providing an easy
> way of thoughtless inflation of commit counts, everything else be damned.

You've made this statement several times over many years.
I don't believe it's true.
I doubt anyone is getting paid per commit.
Who really cares enough to game some stupid little metric?

If it's really a big deal, name some names.  Otherwise please
stop with this dubious argument/point.

> Some of these checks are common-sense, some are
> absolutely arbitrary,

Essentially all of the checkpatch rules are arbitrary.
The compiler doesn't care one way or another.

All the checks exist just to make code appear more consistent.

The conceit being that more consistent code is easier for humans
to read and spot potential defects and possible reuse patterns.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Joe Perches]

On Thu, 2016-09-22 at 13:57 +0200, Jean Delvare wrote:
> Sure. But I'm afraid you keep changing topics and I have no idea where
> you are going. We started with "should there be a space before jump
> labels", then out of nowhere we were discussing the wording of the
> output of checkpatch (how is that related?) and now you pull statistics
> out of your hat, like these numbers imply anything.

No, not out of a hat.  Those are the results of a silly script that
runs checkpatch on every .[ch] kernel file (but not tools/) with:

	--show-types --terse --emacs --strict --no-summary --quiet -f

The magnitude of "ERRORS" is high and it's not necessary or useful
to modify old or obsolete code just to reduce that magnitude.

> checkpatch was called checkPATCH for a reason.

That's why I promote the --force option to limit using checkpatch on
files outside of staging.

https://patchwork.kernel.org/patch/9332205/

Andrew?  Are you going to apply that one day?

> ERROR means that the new code isn't allowed to do that. Period.

Disagree.  The compiler doesn't care.  The value of consistency in
reducing defects is very hard to quantify.

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Joe Perches]

On Thu, 2016-09-22 at 13:57 +0200, Jean Delvare wrote:
> Sure. But I'm afraid you keep changing topics and I have no idea where
> you are going. We started with "should there be a space before jump
> labels", then out of nowhere we were discussing the wording of the
> output of checkpatch (how is that related?) and now you pull statistics
> out of your hat, like these numbers imply anything.

No, not out of a hat.  Those are the results of a silly script that
runs checkpatch on every .[ch] kernel file (but not tools/) with:

	--show-types --terse --emacs --strict --no-summary --quiet -f

The magnitude of "ERRORS" is high and it's not necessary or useful
to modify old or obsolete code just to reduce that magnitude.

> checkpatch was called checkPATCH for a reason.

That's why I promote the --force option to limit using checkpatch on
files outside of staging.

https://patchwork.kernel.org/patch/9332205/

Andrew?  Are you going to apply that one day?

> ERROR means that the new code isn't allowed to do that. Period.

Disagree.  The compiler doesn't care.  The value of consistency in
reducing defects is very hard to quantify.

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jean Delvare]

On Thu, 22 Sep 2016 10:49:47 -0700, Joe Perches wrote:
> On Thu, 2016-09-22 at 13:57 +0200, Jean Delvare wrote:
> > Sure. But I'm afraid you keep changing topics and I have no idea where
> > you are going. We started with "should there be a space before jump
> > labels", then out of nowhere we were discussing the wording of the
> > output of checkpatch (how is that related?) and now you pull statistics
> > out of your hat, like these numbers imply anything.
> 
> No, not out of a hat.  Those are the results of a silly script that
> runs checkpatch on every .[ch] kernel file (but not tools/) with:
> 
> 	--show-types --terse --emacs --strict --no-summary --quiet -f

Silly is the key word here. Just don't do it.

> The magnitude of "ERRORS" is high and it's not necessary or useful
> to modify old or obsolete code just to reduce that magnitude.

I agree. Just don't do it.

> > checkpatch was called checkPATCH for a reason.
> 
> That's why I promote the --force option to limit using checkpatch on
> files outside of staging.
> 
> https://patchwork.kernel.org/patch/9332205/
> 
> Andrew?  Are you going to apply that one day?

I hope not. Looks plain wrong to me. This wont prevents idiots from
being idiots. All it does is make things more difficult for the rest of
us.

> > ERROR means that the new code isn't allowed to do that. Period.
> 
> Disagree.  The compiler doesn't care.

Which is good, because this has nothing to do with the compiler.

> The value of consistency in reducing defects is very hard to quantify.

That's not the only purpose of consistency.

-- 
Jean Delvare
SUSE L3 Support

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jean Delvare]

On Thu, 22 Sep 2016 10:49:47 -0700, Joe Perches wrote:
> On Thu, 2016-09-22 at 13:57 +0200, Jean Delvare wrote:
> > Sure. But I'm afraid you keep changing topics and I have no idea where
> > you are going. We started with "should there be a space before jump
> > labels", then out of nowhere we were discussing the wording of the
> > output of checkpatch (how is that related?) and now you pull statistics
> > out of your hat, like these numbers imply anything.
> 
> No, not out of a hat.  Those are the results of a silly script that
> runs checkpatch on every .[ch] kernel file (but not tools/) with:
> 
> 	--show-types --terse --emacs --strict --no-summary --quiet -f

Silly is the key word here. Just don't do it.

> The magnitude of "ERRORS" is high and it's not necessary or useful
> to modify old or obsolete code just to reduce that magnitude.

I agree. Just don't do it.

> > checkpatch was called checkPATCH for a reason.
> 
> That's why I promote the --force option to limit using checkpatch on
> files outside of staging.
> 
> https://patchwork.kernel.org/patch/9332205/
> 
> Andrew?  Are you going to apply that one day?

I hope not. Looks plain wrong to me. This wont prevents idiots from
being idiots. All it does is make things more difficult for the rest of
us.

> > ERROR means that the new code isn't allowed to do that. Period.
> 
> Disagree.  The compiler doesn't care.

Which is good, because this has nothing to do with the compiler.

> The value of consistency in reducing defects is very hard to quantify.

That's not the only purpose of consistency.

-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jani Nikula]

On Thu, 22 Sep 2016, Jean Delvare <jdelvare@suse.de> 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

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jani Nikula]

On Thu, 22 Sep 2016, Jean Delvare <jdelvare@suse.de> 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

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jean Delvare]

Hi Jani,

On Thu, 22 Sep 2016 13:43:42 +0300, Jani Nikula wrote:
> On Thu, 22 Sep 2016, Jean Delvare <jdelvare@suse.de> wrote:
> > 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.

Fixing old code isn't wrong per se. It's good actually. But only if
done the right way by the right person. I don't think it makes any
sense to use this task as an introduction to kernel development for
newcomers. It doesn't teach them anything about the kernel, really.

-- 
Jean Delvare
SUSE L3 Support

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jean Delvare]

Hi Jani,

On Thu, 22 Sep 2016 13:43:42 +0300, Jani Nikula wrote:
> On Thu, 22 Sep 2016, Jean Delvare <jdelvare@suse.de> wrote:
> > 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.

Fixing old code isn't wrong per se. It's good actually. But only if
done the right way by the right person. I don't think it makes any
sense to use this task as an introduction to kernel development for
newcomers. It doesn't teach them anything about the kernel, really.

-- 
Jean Delvare
SUSE L3 Support

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jani Nikula]

On Thu, 22 Sep 2016, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Jani,
>
> On Thu, 22 Sep 2016 13:43:42 +0300, Jani Nikula wrote:
>> 
>> 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.
>
> Fixing old code isn't wrong per se. It's good actually. But only if
> done the right way by the right person. I don't think it makes any
> sense to use this task as an introduction to kernel development for
> newcomers. It doesn't teach them anything about the kernel, really.

Mostly agreed, though I'd go as far as saying certain classes of
(checkpatch) issues aren't worth fixing, at all, by anyone, except
perhaps while changing the code anyway for some other purpose.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: "CodingStyle: Clarify and complete chapter 7" in docs-next

[Jani Nikula]

On Thu, 22 Sep 2016, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Jani,
>
> On Thu, 22 Sep 2016 13:43:42 +0300, Jani Nikula wrote:
>> 
>> 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.
>
> Fixing old code isn't wrong per se. It's good actually. But only if
> done the right way by the right person. I don't think it makes any
> sense to use this task as an introduction to kernel development for
> newcomers. It doesn't teach them anything about the kernel, really.

Mostly agreed, though I'd go as far as saying certain classes of
(checkpatch) issues aren't worth fixing, at all, by anyone, except
perhaps while changing the code anyway for some other purpose.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center