Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Jean Delvare]

Hi Peter,

On Tue, 6 Sep 2016 16:47:56 +0200, Peter Zijlstra wrote:
> On Tue, Sep 06, 2016 at 04:34:13PM +0200, Jean Delvare wrote:
> > > [diff "default"]
> > > xfuncname = "^[[:alpha:]$_].*[^:]$"
> > 
> > OK, I see. As mentioned somewhere else, it fails for labels which have
> > comments. 
> 
> Heh, There's labels that have comments?

Yes, 43.

> > My worry is that you recommending it to contributors on a individual
> > and opportunity basis, doesn't scale. Basing coding style
> > recommendations on a personal quirk doesn't strike me as the best idea
> > ever in the long run.
> 
> Don't care too much, I simply will not take any patch that adds stupid
> spaces :-)
> 
> > While looking at the syntax of your example, I have found something
> > which looks more promising. git already has predefined xfuncname
> > definitions for various languages, including C. These can be enabled
> > based on file name patterns via gitattributes. The
> > following .gitattribute file placed at the root of the kernel source
> > tree achieves what you want:
> > 
> > *.c   diff=cpp
> > *.h   diff=cpp
> > 
> > The major difference between git config and gitattributes is that the
> > latter can be part of the project itself, just like gitignore. So we
> > could just push that .gitattribute file upstream, and then labels
> > without leading spaces would no longer be a problem, at least within
> > git.
> 
> Works for me,

OK, I'll send a patch now.

> and last time this came up Linus agreed with the
> "whitespace before labels is stupid" thing. Although I cannot find a
> link to that just now.

Murphy's law applies, you can never find links again the day you
desperately need them. If you ever get your hands on that one again,
please let me know, I really would like to read that post.

> > It would still be a problem for me as an inveterate quilt user,
> 
> Add the below to your .quiltrc or environment:
> 
> QUILT_DIFF_OPTS="-F ^[[:alpha:]\$_].*[^:]\$"

I didn't know this option existed, thanks for the pointer.

Now I'm sure I won't try to get the behavior of GNU diff option -p
changed, as I think I know what the answer would be.

> Same caveat about labels with comments, but then I'd not take a patch
> doing that in the first place.

I'll improve the regular expression if I ever have to (I don't think a
function declaration can have a colon anywhere?), but I'm happy if it
works in 99.9 % of the cases, thank sagain.

-- 
Jean Delvare
SUSE L3 Support

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Jean Delvare]

Hi Peter,

On Tue, 6 Sep 2016 16:47:56 +0200, Peter Zijlstra wrote:
> On Tue, Sep 06, 2016 at 04:34:13PM +0200, Jean Delvare wrote:
> > > [diff "default"]
> > > xfuncname = "^[[:alpha:]$_].*[^:]$"
> > 
> > OK, I see. As mentioned somewhere else, it fails for labels which have
> > comments. 
> 
> Heh, There's labels that have comments?

Yes, 43.

> > My worry is that you recommending it to contributors on a individual
> > and opportunity basis, doesn't scale. Basing coding style
> > recommendations on a personal quirk doesn't strike me as the best idea
> > ever in the long run.
> 
> Don't care too much, I simply will not take any patch that adds stupid
> spaces :-)
> 
> > While looking at the syntax of your example, I have found something
> > which looks more promising. git already has predefined xfuncname
> > definitions for various languages, including C. These can be enabled
> > based on file name patterns via gitattributes. The
> > following .gitattribute file placed at the root of the kernel source
> > tree achieves what you want:
> > 
> > *.c   diff=cpp
> > *.h   diff=cpp
> > 
> > The major difference between git config and gitattributes is that the
> > latter can be part of the project itself, just like gitignore. So we
> > could just push that .gitattribute file upstream, and then labels
> > without leading spaces would no longer be a problem, at least within
> > git.
> 
> Works for me,

OK, I'll send a patch now.

> and last time this came up Linus agreed with the
> "whitespace before labels is stupid" thing. Although I cannot find a
> link to that just now.

Murphy's law applies, you can never find links again the day you
desperately need them. If you ever get your hands on that one again,
please let me know, I really would like to read that post.

> > It would still be a problem for me as an inveterate quilt user,
> 
> Add the below to your .quiltrc or environment:
> 
> QUILT_DIFF_OPTS="-F ^[[:alpha:]\$_].*[^:]\$"

I didn't know this option existed, thanks for the pointer.

Now I'm sure I won't try to get the behavior of GNU diff option -p
changed, as I think I know what the answer would be.

> Same caveat about labels with comments, but then I'd not take a patch
> doing that in the first place.

I'll improve the regular expression if I ever have to (I don't think a
function declaration can have a colon anywhere?), but I'm happy if it
works in 99.9 % of the cases, thank sagain.

-- 
Jean Delvare
SUSE L3 Support

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Joe Perches]

On Tue, 2016-09-06 at 16:47 +0200, Peter Zijlstra wrote:
> On Tue, Sep 06, 2016 at 04:34:13PM +0200, Jean Delvare wrote:
> > > [diff "default"]
> > >         xfuncname = "^[[:alpha:]$_].*[^:]$"
> > OK, I see. As mentioned somewhere else, it fails for labels which have
> > comments. 
> Heh, There's labels that have comments?

Only a few dozen.

The pattern with the perl-like $_ took me a depressingly
long time to parse followed by a self forehead slap.

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Joe Perches]

On Tue, 2016-09-06 at 16:47 +0200, Peter Zijlstra wrote:
> On Tue, Sep 06, 2016 at 04:34:13PM +0200, Jean Delvare wrote:
> > > [diff "default"]
> > >         xfuncname = "^[[:alpha:]$_].*[^:]$"
> > OK, I see. As mentioned somewhere else, it fails for labels which have
> > comments. 
> Heh, There's labels that have comments?

Only a few dozen.

The pattern with the perl-like $_ took me a depressingly
long time to parse followed by a self forehead slap.

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Peter Zijlstra]

On Tue, Sep 06, 2016 at 04:34:13PM +0200, Jean Delvare wrote:
> > [diff "default"]
> > xfuncname = "^[[:alpha:]$_].*[^:]$"
> 
> OK, I see. As mentioned somewhere else, it fails for labels which have
> comments. 

Heh, There's labels that have comments?

> My worry is that you recommending it to contributors on a individual
> and opportunity basis, doesn't scale. Basing coding style
> recommendations on a personal quirk doesn't strike me as the best idea
> ever in the long run.

Don't care too much, I simply will not take any patch that adds stupid
spaces :-)

> While looking at the syntax of your example, I have found something
> which looks more promising. git already has predefined xfuncname
> definitions for various languages, including C. These can be enabled
> based on file name patterns via gitattributes. The
> following .gitattribute file placed at the root of the kernel source
> tree achieves what you want:
> 
> *.c   diff=cpp
> *.h   diff=cpp
> 
> The major difference between git config and gitattributes is that the
> latter can be part of the project itself, just like gitignore. So we
> could just push that .gitattribute file upstream, and then labels
> without leading spaces would no longer be a problem, at least within
> git.

Works for me, and last time this came up Linus agreed with the
"whitespace before labels is stupid" thing. Although I cannot find a
link to that just now.

> It would still be a problem for me as an inveterate quilt user,

Add the below to your .quiltrc or environment:

QUILT_DIFF_OPTS="-F ^[[:alpha:]\$_].*[^:]\$"

Same caveat about labels with comments, but then I'd not take a patch
doing that in the first place.

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Peter Zijlstra]

On Tue, Sep 06, 2016 at 04:34:13PM +0200, Jean Delvare wrote:
> > [diff "default"]
> > xfuncname = "^[[:alpha:]$_].*[^:]$"
> 
> OK, I see. As mentioned somewhere else, it fails for labels which have
> comments. 

Heh, There's labels that have comments?

> My worry is that you recommending it to contributors on a individual
> and opportunity basis, doesn't scale. Basing coding style
> recommendations on a personal quirk doesn't strike me as the best idea
> ever in the long run.

Don't care too much, I simply will not take any patch that adds stupid
spaces :-)

> While looking at the syntax of your example, I have found something
> which looks more promising. git already has predefined xfuncname
> definitions for various languages, including C. These can be enabled
> based on file name patterns via gitattributes. The
> following .gitattribute file placed at the root of the kernel source
> tree achieves what you want:
> 
> *.c   diff=cpp
> *.h   diff=cpp
> 
> The major difference between git config and gitattributes is that the
> latter can be part of the project itself, just like gitignore. So we
> could just push that .gitattribute file upstream, and then labels
> without leading spaces would no longer be a problem, at least within
> git.

Works for me, and last time this came up Linus agreed with the
"whitespace before labels is stupid" thing. Although I cannot find a
link to that just now.

> It would still be a problem for me as an inveterate quilt user,

Add the below to your .quiltrc or environment:

QUILT_DIFF_OPTS="-F ^[[:alpha:]\$_].*[^:]\$"

Same caveat about labels with comments, but then I'd not take a patch
doing that in the first place.

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Jean Delvare]

Hi Peter,

On Mon, 5 Sep 2016 13:58:38 +0200, Peter Zijlstra wrote:
> On Mon, Sep 05, 2016 at 01:54:45PM +0200, Jean Delvare wrote:
> > On Mon, 5 Sep 2016 13:37:04 +0200, Peter Zijlstra wrote:
> > > I have it in my local .gitconfig, and recommend it to people who send me
> > > patches.
> > 
> > What does it look like, please?
> 
> [diff "default"]
> xfuncname = "^[[:alpha:]$_].*[^:]$"

OK, I see. As mentioned somewhere else, it fails for labels which have
comments. I was also surprised by the $ but apparently it's valid in
identifiers for at least some incarnations of C o.O

My worry is that you recommending it to contributors on a individual
and opportunity basis, doesn't scale. Basing coding style
recommendations on a personal quirk doesn't strike me as the best idea
ever in the long run.

The reason why I proposed an update to CodingStyle regarding this topic
was precisely to avoid having to repeat the same to contributors, like
you do (although our recommendations are different.)

While looking at the syntax of your example, I have found something
which looks more promising. git already has predefined xfuncname
definitions for various languages, including C. These can be enabled
based on file name patterns via gitattributes. The
following .gitattribute file placed at the root of the kernel source
tree achieves what you want:

*.c   diff=cpp
*.h   diff=cpp

The major difference between git config and gitattributes is that the
latter can be part of the project itself, just like gitignore. So we
could just push that .gitattribute file upstream, and then labels
without leading spaces would no longer be a problem, at least within
git. It would still be a problem for me as an inveterate quilt user, at
least until GNU diff gets "fixed." Which I did not even try, as I'm not
sure if upstream really considers this a bug in the first place.

And just for completeness, git's "cpp" predefined pattern doesn't
actually support $ as part of identifiers.

-- 
Jean Delvare
SUSE L3 Support

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Jean Delvare]

Hi Peter,

On Mon, 5 Sep 2016 13:58:38 +0200, Peter Zijlstra wrote:
> On Mon, Sep 05, 2016 at 01:54:45PM +0200, Jean Delvare wrote:
> > On Mon, 5 Sep 2016 13:37:04 +0200, Peter Zijlstra wrote:
> > > I have it in my local .gitconfig, and recommend it to people who send me
> > > patches.
> > 
> > What does it look like, please?
> 
> [diff "default"]
> xfuncname = "^[[:alpha:]$_].*[^:]$"

OK, I see. As mentioned somewhere else, it fails for labels which have
comments. I was also surprised by the $ but apparently it's valid in
identifiers for at least some incarnations of C o.O

My worry is that you recommending it to contributors on a individual
and opportunity basis, doesn't scale. Basing coding style
recommendations on a personal quirk doesn't strike me as the best idea
ever in the long run.

The reason why I proposed an update to CodingStyle regarding this topic
was precisely to avoid having to repeat the same to contributors, like
you do (although our recommendations are different.)

While looking at the syntax of your example, I have found something
which looks more promising. git already has predefined xfuncname
definitions for various languages, including C. These can be enabled
based on file name patterns via gitattributes. The
following .gitattribute file placed at the root of the kernel source
tree achieves what you want:

*.c   diff=cpp
*.h   diff=cpp

The major difference between git config and gitattributes is that the
latter can be part of the project itself, just like gitignore. So we
could just push that .gitattribute file upstream, and then labels
without leading spaces would no longer be a problem, at least within
git. It would still be a problem for me as an inveterate quilt user, at
least until GNU diff gets "fixed." Which I did not even try, as I'm not
sure if upstream really considers this a bug in the first place.

And just for completeness, git's "cpp" predefined pattern doesn't
actually support $ as part of identifiers.

-- 
Jean Delvare
SUSE L3 Support

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Peter Zijlstra]

On Mon, Sep 05, 2016 at 01:54:45PM +0200, Jean Delvare wrote:
> On Mon, 5 Sep 2016 13:37:04 +0200, Peter Zijlstra wrote:
> > On Mon, Sep 05, 2016 at 01:07:37PM +0200, Jean Delvare wrote:
> > > Now I see in http://patchwork.ozlabs.org/patch/664966/ that Peter
> > > Zijlstra reportedly changed the behavior of "diff -p" so that it
> > > handles unindented C labels nicely. If this actually happens, it could
> > > change my point of view. However I can't find this commit in upstream
> > > diffutils. Peter, can you please clarify the situation? Is it just a
> > > local hack on your own instance of "diff"?
> > 
> > I have it in my local .gitconfig, and recommend it to people who send me
> > patches.
> 
> What does it look like, please?

[diff "default"]
xfuncname = "^[[:alpha:]$_].*[^:]$"
[core]
abbrev = 12
[alias]
one = show -s --pretty='format:%h (\"%s\")'
[rerere]
enable = true
enabled = true
autoupdate = true

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Peter Zijlstra]

On Mon, Sep 05, 2016 at 01:54:45PM +0200, Jean Delvare wrote:
> On Mon, 5 Sep 2016 13:37:04 +0200, Peter Zijlstra wrote:
> > On Mon, Sep 05, 2016 at 01:07:37PM +0200, Jean Delvare wrote:
> > > Now I see in http://patchwork.ozlabs.org/patch/664966/ that Peter
> > > Zijlstra reportedly changed the behavior of "diff -p" so that it
> > > handles unindented C labels nicely. If this actually happens, it could
> > > change my point of view. However I can't find this commit in upstream
> > > diffutils. Peter, can you please clarify the situation? Is it just a
> > > local hack on your own instance of "diff"?
> > 
> > I have it in my local .gitconfig, and recommend it to people who send me
> > patches.
> 
> What does it look like, please?

[diff "default"]
xfuncname = "^[[:alpha:]$_].*[^:]$"
[core]
abbrev = 12
[alias]
one = show -s --pretty='format:%h (\"%s\")'
[rerere]
enable = true
enabled = true
autoupdate = true

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Jean Delvare]

On Mon, 5 Sep 2016 13:37:04 +0200, Peter Zijlstra wrote:
> On Mon, Sep 05, 2016 at 01:07:37PM +0200, Jean Delvare wrote:
> > Now I see in http://patchwork.ozlabs.org/patch/664966/ that Peter
> > Zijlstra reportedly changed the behavior of "diff -p" so that it
> > handles unindented C labels nicely. If this actually happens, it could
> > change my point of view. However I can't find this commit in upstream
> > diffutils. Peter, can you please clarify the situation? Is it just a
> > local hack on your own instance of "diff"?
> 
> I have it in my local .gitconfig, and recommend it to people who send me
> patches.

What does it look like, please?

> I've never tried to get diffutils fixed, although maybe I should.

If you don't want one-space-indented labels to become the norm, then
yes you should.

-- 
Jean Delvare
SUSE L3 Support

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Jean Delvare]

On Mon, 5 Sep 2016 13:37:04 +0200, Peter Zijlstra wrote:
> On Mon, Sep 05, 2016 at 01:07:37PM +0200, Jean Delvare wrote:
> > Now I see in http://patchwork.ozlabs.org/patch/664966/ that Peter
> > Zijlstra reportedly changed the behavior of "diff -p" so that it
> > handles unindented C labels nicely. If this actually happens, it could
> > change my point of view. However I can't find this commit in upstream
> > diffutils. Peter, can you please clarify the situation? Is it just a
> > local hack on your own instance of "diff"?
> 
> I have it in my local .gitconfig, and recommend it to people who send me
> patches.

What does it look like, please?

> I've never tried to get diffutils fixed, although maybe I should.

If you don't want one-space-indented labels to become the norm, then
yes you should.

-- 
Jean Delvare
SUSE L3 Support

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Peter Zijlstra]

On Mon, Sep 05, 2016 at 01:07:37PM +0200, Jean Delvare wrote:
> Now I see in http://patchwork.ozlabs.org/patch/664966/ that Peter
> Zijlstra reportedly changed the behavior of "diff -p" so that it
> handles unindented C labels nicely. If this actually happens, it could
> change my point of view. However I can't find this commit in upstream
> diffutils. Peter, can you please clarify the situation? Is it just a
> local hack on your own instance of "diff"?

I have it in my local .gitconfig, and recommend it to people who send me
patches.

I've never tried to get diffutils fixed, although maybe I should.

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Peter Zijlstra]

On Mon, Sep 05, 2016 at 01:07:37PM +0200, Jean Delvare wrote:
> Now I see in http://patchwork.ozlabs.org/patch/664966/ that Peter
> Zijlstra reportedly changed the behavior of "diff -p" so that it
> handles unindented C labels nicely. If this actually happens, it could
> change my point of view. However I can't find this commit in upstream
> diffutils. Peter, can you please clarify the situation? Is it just a
> local hack on your own instance of "diff"?

I have it in my local .gitconfig, and recommend it to people who send me
patches.

I've never tried to get diffutils fixed, although maybe I should.

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Jean Delvare]

Hi Daniel,

Preliminary note: the SNR of Markus Elfring is incredibly low. I advise
you just ignore him.

On Sun, 04 Sep 2016 11:56:58 +0200, Daniel Borkmann wrote:
> I don't want to drag this thread onwards for (way) too long, but clearly "it 
> is
> advised to indent labels with a single space (not tab)" (from diff in above 
> commit)
> doesn't really reflect the majority of kernel practice we have in-tree today 
> and
> actually rather adds more confusion than any clarification whatsoever:
> 
>$ git grep -n "^\ [a-z_]*:" -- '*.[ch]' | wc -l
>4919
>$ git grep -n "^[a-z_]*:" -- '*.[ch]' | wc -l
>54686

Well the documentation update in question has not hit mainline yet, so
it's not really surprising.

> A CodingStyle document should document what's regarded as a general consensus 
> of
> kernel coding practices, and thus should represent the /majority/ of coding 
> style,

I beg to disagree. Recommendations are not meant to document what
people are currently doing but what we think they should be doing. By
your reasoning, we would have killed all the devm infrastructure,
because at some point in time (and it might still be the case) most
drivers were not using it.

There is a rationale for the leading space, it is given in the patch,
but sadly you decided to not quote it above.

> which (if I didn't screw up my git-grep line completely) above 9% does not 
> really
> reflect at all. So, new folks starting with kernel hacking reading this are 
> rather

Your grep patterns are slightly inaccurate. Space doesn't need to be
escaped, labels can use capital letters, and except for the first
character, digits are accepted too. Also to be completely fair, you
should also count the labels which are intended using tabs. But it
doesn't change the balance noticeably anyway, so no big deal.

> misguided, and code-wise it just adds up to have more inconsistencies from new
> patches, or worse, have noisy patches (like this one) flying around that try 
> to
> brute-force everything into this advice.

Guys who like to waste our time with pointless patches will always find
a way to do that, sadly, so I don't think this point is relevant.

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.

My assumption was that the behavior of "diff -p" would never change, as
despite the language-specificity of its long option name, it seemed too
generic to be loaded with C-specific rules.

Now I see in http://patchwork.ozlabs.org/patch/664966/ that Peter
Zijlstra reportedly changed the behavior of "diff -p" so that it
handles unindented C labels nicely. If this actually happens, it could
change my point of view. However I can't find this commit in upstream
diffutils. Peter, can you please clarify the situation? Is it just a
local hack on your own instance of "diff"?

Even if upstream diff is ever changed, it will take some time until new
versions propagate to all developers. And until this happens, my
preference for one-space-indented labels will remain.

Also git has its own implementation of "diff", so any change in the
behavior of GNU diff's -p and/or --show-c-function options should be
reflected there as well for consistency.

-- 
Jean Delvare
SUSE L3 Support

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Jean Delvare]

Hi Daniel,

Preliminary note: the SNR of Markus Elfring is incredibly low. I advise
you just ignore him.

On Sun, 04 Sep 2016 11:56:58 +0200, Daniel Borkmann wrote:
> I don't want to drag this thread onwards for (way) too long, but clearly "it 
> is
> advised to indent labels with a single space (not tab)" (from diff in above 
> commit)
> doesn't really reflect the majority of kernel practice we have in-tree today 
> and
> actually rather adds more confusion than any clarification whatsoever:
> 
>$ git grep -n "^\ [a-z_]*:" -- '*.[ch]' | wc -l
>4919
>$ git grep -n "^[a-z_]*:" -- '*.[ch]' | wc -l
>54686

Well the documentation update in question has not hit mainline yet, so
it's not really surprising.

> A CodingStyle document should document what's regarded as a general consensus 
> of
> kernel coding practices, and thus should represent the /majority/ of coding 
> style,

I beg to disagree. Recommendations are not meant to document what
people are currently doing but what we think they should be doing. By
your reasoning, we would have killed all the devm infrastructure,
because at some point in time (and it might still be the case) most
drivers were not using it.

There is a rationale for the leading space, it is given in the patch,
but sadly you decided to not quote it above.

> which (if I didn't screw up my git-grep line completely) above 9% does not 
> really
> reflect at all. So, new folks starting with kernel hacking reading this are 
> rather

Your grep patterns are slightly inaccurate. Space doesn't need to be
escaped, labels can use capital letters, and except for the first
character, digits are accepted too. Also to be completely fair, you
should also count the labels which are intended using tabs. But it
doesn't change the balance noticeably anyway, so no big deal.

> misguided, and code-wise it just adds up to have more inconsistencies from new
> patches, or worse, have noisy patches (like this one) flying around that try 
> to
> brute-force everything into this advice.

Guys who like to waste our time with pointless patches will always find
a way to do that, sadly, so I don't think this point is relevant.

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.

My assumption was that the behavior of "diff -p" would never change, as
despite the language-specificity of its long option name, it seemed too
generic to be loaded with C-specific rules.

Now I see in http://patchwork.ozlabs.org/patch/664966/ that Peter
Zijlstra reportedly changed the behavior of "diff -p" so that it
handles unindented C labels nicely. If this actually happens, it could
change my point of view. However I can't find this commit in upstream
diffutils. Peter, can you please clarify the situation? Is it just a
local hack on your own instance of "diff"?

Even if upstream diff is ever changed, it will take some time until new
versions propagate to all developers. And until this happens, my
preference for one-space-indented labels will remain.

Also git has its own implementation of "diff", so any change in the
behavior of GNU diff's -p and/or --show-c-function options should be
reflected there as well for consistency.

-- 
Jean Delvare
SUSE L3 Support

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Daniel Borkmann]

On 09/04/2016 09:20 AM, SF Markus Elfring wrote:

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51


[ + Jonathan for above commit in linux-next ]


You seem to lack understanding of the difference between absolute
requirements and "advice".

As Sparc maintainer I can choose to not take this "advice",
and I so choose to do so.


Your conclusion can be fine in principle.

I am just curious on how much further software development "fun" the recent 
update
by a topic like "CodingStyle: Clarify and complete chapter 7" will trigger.


I don't want to drag this thread onwards for (way) too long, but clearly "it is
advised to indent labels with a single space (not tab)" (from diff in above 
commit)
doesn't really reflect the majority of kernel practice we have in-tree today and
actually rather adds more confusion than any clarification whatsoever:

  $ git grep -n "^\ [a-z_]*:" -- '*.[ch]' | wc -l
  4919
  $ git grep -n "^[a-z_]*:" -- '*.[ch]' | wc -l
  54686

A CodingStyle document should document what's regarded as a general consensus of
kernel coding practices, and thus should represent the /majority/ of coding 
style,
which (if I didn't screw up my git-grep line completely) above 9% does not 
really
reflect at all. So, new folks starting with kernel hacking reading this are 
rather
misguided, and code-wise it just adds up to have more inconsistencies from new
patches, or worse, have noisy patches (like this one) flying around that try to
brute-force everything into this advice.

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[Daniel Borkmann]

On 09/04/2016 09:20 AM, SF Markus Elfring wrote:

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51


[ + Jonathan for above commit in linux-next ]


You seem to lack understanding of the difference between absolute
requirements and "advice".

As Sparc maintainer I can choose to not take this "advice",
and I so choose to do so.


Your conclusion can be fine in principle.

I am just curious on how much further software development "fun" the recent 
update
by a topic like "CodingStyle: Clarify and complete chapter 7" will trigger.


I don't want to drag this thread onwards for (way) too long, but clearly "it is
advised to indent labels with a single space (not tab)" (from diff in above 
commit)
doesn't really reflect the majority of kernel practice we have in-tree today and
actually rather adds more confusion than any clarification whatsoever:

  $ git grep -n "^\ [a-z_]*:" -- '*.[ch]' | wc -l
  4919
  $ git grep -n "^[a-z_]*:" -- '*.[ch]' | wc -l
  54686

A CodingStyle document should document what's regarded as a general consensus of
kernel coding practices, and thus should represent the /majority/ of coding 
style,
which (if I didn't screw up my git-grep line completely) above 9% does not 
really
reflect at all. So, new folks starting with kernel hacking reading this are 
rather
misguided, and code-wise it just adds up to have more inconsistencies from new
patches, or worse, have noisy patches (like this one) flying around that try to
brute-force everything into this advice.

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[SF Markus Elfring]

> It's not because I find improvements "uncomfortable", but rather it's
> because your changes are not seen as improvements in the first place.

What is your software development opinion for the update step
"[1/4] sparc: bpf_jit: Use kmalloc_array() in bpf_jit_compile()"
from this small patch series?

Regards,
Markus

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[SF Markus Elfring]

> It's not because I find improvements "uncomfortable", but rather it's
> because your changes are not seen as improvements in the first place.

What is your software development opinion for the update step
"[1/4] sparc: bpf_jit: Use kmalloc_array() in bpf_jit_compile()"
from this small patch series?

Regards,
Markus

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[SF Markus Elfring]

>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51
> 
> You seem to lack understanding of the difference between absolute
> requirements and "advice".
> 
> As Sparc maintainer I can choose to not take this "advice",
> and I so choose to do so.

Your conclusion can be fine in principle.

I am just curious on how much further software development "fun" the recent 
update
by a topic like "CodingStyle: Clarify and complete chapter 7" will trigger.


> If you want to be completely ignored by me,

I hope that this action does not need to happen.


> then keep arguing the way you are right now.

I guess that I will stumble on more software improvement opportunities
you find harder to become comfortable with.

Regards,
Markus

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[SF Markus Elfring]

>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51
> 
> You seem to lack understanding of the difference between absolute
> requirements and "advice".
> 
> As Sparc maintainer I can choose to not take this "advice",
> and I so choose to do so.

Your conclusion can be fine in principle.

I am just curious on how much further software development "fun" the recent 
update
by a topic like "CodingStyle: Clarify and complete chapter 7" will trigger.


> If you want to be completely ignored by me,

I hope that this action does not need to happen.


> then keep arguing the way you are right now.

I guess that I will stumble on more software improvement opportunities
you find harder to become comfortable with.

Regards,
Markus

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[David Miller]

From: SF Markus Elfring 
Date: Sun, 4 Sep 2016 09:20:55 +0200

> I guess that I will stumble on more software improvement opportunities
> you find harder to become comfortable with.

Improvement is a matter of opinion.  So your statement assumes that
your changes are an improvement, and everyone in this thread clearly
disagrees with that.

This is why everything you are doing here is so irritating.

It's not because I find improvements "uncomfortable", but rather it's
because your changes are not seen as improvements in the first place.

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[David Miller]

From: SF Markus Elfring 
Date: Sun, 4 Sep 2016 09:20:55 +0200

> I guess that I will stumble on more software improvement opportunities
> you find harder to become comfortable with.

Improvement is a matter of opinion.  So your statement assumes that
your changes are an improvement, and everyone in this thread clearly
disagrees with that.

This is why everything you are doing here is so irritating.

It's not because I find improvements "uncomfortable", but rather it's
because your changes are not seen as improvements in the first place.

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[David Miller]

From: SF Markus Elfring 
Date: Sun, 4 Sep 2016 08:50:20 +0200

>>> NAK, just noise.
>> 
>> And frankly I hate that leading space.
> 
> Would you like to comment the recent update of the document "CodingStyle" any 
> more?
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51

You seem to lack understanding of the difference between absolute
requirements and "advice".

As Sparc maintainer I can choose to not take this "advice", and I so
choose to do so.

If you want to be completely ignored by me, then keep arguing the way
you are right now.

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[David Miller]

From: SF Markus Elfring 
Date: Sun, 4 Sep 2016 08:50:20 +0200

>>> NAK, just noise.
>> 
>> And frankly I hate that leading space.
> 
> Would you like to comment the recent update of the document "CodingStyle" any 
> more?
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51

You seem to lack understanding of the difference between absolute
requirements and "advice".

As Sparc maintainer I can choose to not take this "advice", and I so
choose to do so.

If you want to be completely ignored by me, then keep arguing the way
you are right now.

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[SF Markus Elfring]

>> NAK, just noise.
> 
> And frankly I hate that leading space.

Would you like to comment the recent update of the document "CodingStyle" any 
more?
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51

Regards,
Markus

Re: sparc: bpf_jit: Rename jump labels in bpf_jit_compile()

[SF Markus Elfring]

>> NAK, just noise.
> 
> And frankly I hate that leading space.

Would you like to comment the recent update of the document "CodingStyle" any 
more?
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51

Regards,
Markus