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())

2016-09-20 Thread Julia Lawall 


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: Adjust the position of a jump label in rbd_header_from_disk())

2016-09-20 Thread Julia Lawall 


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: Adjust the position of a jump label in rbd_header_from_disk())

2016-09-20 Thread 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: Adjust the position of a jump label in rbd_header_from_disk())

2016-09-20 Thread 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: Adjust the position of a jump label in rbd_header_from_disk())

2016-09-19 Thread 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 majord...@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())

2016-09-19 Thread 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 majord...@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())

2016-09-19 Thread 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())

2016-09-19 Thread 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())

2016-09-19 Thread 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())

2016-09-19 Thread 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.

"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())

2016-09-19 Thread Ilya Dryomov 
On Mon, Sep 19, 2016 at 11:37 AM, Jean Delvare  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=147325166209844=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 the position of a jump label in rbd_header_from_disk())

2016-09-19 Thread Ilya Dryomov 
On Mon, Sep 19, 2016 at 11:37 AM, Jean Delvare  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=147325166209844=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