Linux kernel development

Ingo Molnar | 16 Apr 2012 11:17

Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts


This recent checkpatch commit, added in v3.4-rc1:

  aad4f6149831 checkpatch: add --strict tests for braces, comments and casts

made the default checkpatch run complain about the following 
perfectly fine multi-line comment block:

+               rdp->qlen_lazy = 0;
+               rdp->qlen = 0;
+
+               /*
+                * Adopt all callbacks.  The outgoing CPU was in no shape
+                * to advance them, so make them all go through a full
+                * grace period.
+                */
+               *receive_rdp->nxttail[RCU_NEXT_TAIL] = rdp->nxtlist;

With:

 CHECK: Don't begin block comments with only a /* line, use /* comment...

#80: FILE: kernel/rcutree.c:1428:
+
+		/*

Which is an obviously bogus warning: the comment is perfectly 
fine and it has the form that Documentation/CodingStyle 
specifically recommends:

(Continue reading)

Randy Dunlap | 16 Apr 2012 21:44

Re: Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts

On 04/16/2012 02:17 AM, Ingo Molnar wrote:

> 
> This recent checkpatch commit, added in v3.4-rc1:
> 
>   aad4f6149831 checkpatch: add --strict tests for braces, comments and casts
> 
> made the default checkpatch run complain about the following 
> perfectly fine multi-line comment block:
> 
> +               rdp->qlen_lazy = 0;
> +               rdp->qlen = 0;
> +
> +               /*
> +                * Adopt all callbacks.  The outgoing CPU was in no shape
> +                * to advance them, so make them all go through a full
> +                * grace period.
> +                */
> +               *receive_rdp->nxttail[RCU_NEXT_TAIL] = rdp->nxtlist;
> 
> With:
> 
>  CHECK: Don't begin block comments with only a /* line, use /* comment...
> 
> #80: FILE: kernel/rcutree.c:1428:
> +
> +		/*
> 
> Which is an obviously bogus warning: the comment is perfectly 
> fine and it has the form that Documentation/CodingStyle

(Continue reading)

Joe Perches | 16 Apr 2012 21:32

[PATCH] checkpatch: revert --strict test for net/ and drivers/net block comment style

---
On Mon, 2012-04-16 at 12:44 -0700, Randy Dunlap wrote:
> On 04/16/2012 02:17 AM, Ingo Molnar wrote:
> >   aad4f6149831 checkpatch: add --strict tests for braces, comments and casts
> > made the default checkpatch run complain about the following 
> > perfectly fine multi-line comment block:

No, it didn't.

It only made checkpatch complain if --strict was
added to the command line arguments.

I don't care about the comment style, it is/was
David Miller's preferred style for drivers/net and
net/

I think a straight revert isn't appropriate.

Here's one that just removes the comment check.

Joe Perches | 16 Apr 2012 21:35

[PATCH] checkpatch: revert --strict test for net/ and drivers/net block comment style

Revert the --strict test for the old preferred block
comment style in drivers/net and net/

Signed-off-by: Joe Perches <joe <at> perches.com>

---

grumble.  Don't reply to email from internet cafes.

On Mon, 2012-04-16 at 12:44 -0700, Randy Dunlap wrote:
> On 04/16/2012 02:17 AM, Ingo Molnar wrote:
> >   aad4f6149831 checkpatch: add --strict tests for braces, comments and casts
> > made the default checkpatch run complain about the following 
> > perfectly fine multi-line comment block:

No, it didn't.

It only made checkpatch complain if --strict was
added to the command line arguments.

I don't care about the comment style, it is/was
David Miller's preferred style for drivers/net and
net/

I think a straight revert isn't appropriate.

Here's one that just removes the comment check.

 scripts/checkpatch.pl |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

(Continue reading)

Ingo Molnar | 16 Apr 2012 22:28

Re: [PATCH] checkpatch: revert --strict test for net/ and drivers/net block comment style


* Joe Perches <joe <at> perches.com> wrote:

> Revert the --strict test for the old preferred block
> comment style in drivers/net and net/

Emphatic NAK.

Thanks,

	Ingo

Linus Torvalds | 16 Apr 2012 22:33

Re: [PATCH] checkpatch: revert --strict test for net/ and drivers/net block comment style

On Mon, Apr 16, 2012 at 1:28 PM, Ingo Molnar <mingo <at> kernel.org> wrote:
>
> * Joe Perches <joe <at> perches.com> wrote:
>
>> Revert the --strict test for the old preferred block
>> comment style in drivers/net and net/
>
> Emphatic NAK.

Ingo, I don't think you understood. That test reverts the thing you
wanted reverted wrt the block comments.

It does not revert the *other* things in that patch (testing for empty
braces etc).

So Joe's patch seems to be exactly what you asked for,

                      Linus

Ingo Molnar | 17 Apr 2012 12:07

Re: [PATCH] checkpatch: revert --strict test for net/ and drivers/net block comment style


* Linus Torvalds <torvalds <at> linux-foundation.org> wrote:

> On Mon, Apr 16, 2012 at 1:28 PM, Ingo Molnar <mingo <at> kernel.org> wrote:
> >
> > * Joe Perches <joe <at> perches.com> wrote:
> >
> >> Revert the --strict test for the old preferred block
> >> comment style in drivers/net and net/
> >
> > Emphatic NAK.
> 
> Ingo, I don't think you understood. That test reverts the thing you
> wanted reverted wrt the block comments.

Yeah, I was confused: I judged it by the title and description 
and assumed it was only doing it for drivers/net/ and net/.

But in reality it just does a wholesale revert of this check 
with no net/ and drivers/net/ specificity to it, so it's 
perfectly fine to me:

Acked-by: Ingo Molnar <mingo <at> elte.hu>

Thanks,

	Ingo

Joe Perches | 26 Apr 2012 00:42

Re: [PATCH] checkpatch: revert --strict test for net/ and drivers/net block comment style

On Tue, 2012-04-17 at 12:07 +0200, Ingo Molnar wrote:
> Yeah, I was confused: I judged it by the title and description 
> and assumed it was only doing it for drivers/net/ and net/.

If you don't read the text, no amount of
geometric layout symmetry will clarify it.

David Miller | 16 Apr 2012 20:27

Re: Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts

From: Ingo Molnar <mingo <at> kernel.org>
Date: Mon, 16 Apr 2012 11:17:20 +0200

> 
> This recent checkpatch commit, added in v3.4-rc1:
> 
>   aad4f6149831 checkpatch: add --strict tests for braces, comments and casts
> 
> made the default checkpatch run complain about the following 
> perfectly fine multi-line comment block:
> 
> +               rdp->qlen_lazy = 0;
> +               rdp->qlen = 0;
> +
> +               /*
> +                * Adopt all callbacks.  The outgoing CPU was in no shape
> +                * to advance them, so make them all go through a full
> +                * grace period.
> +                */
> +               *receive_rdp->nxttail[RCU_NEXT_TAIL] = rdp->nxtlist;
> 

I would like to formally propose that we change CodingStyle so that
our comments are of the form:

	/* XXX
	 * YYY
	 */

So that we can save one line of vertical space.

(Continue reading)

Linus Torvalds | 16 Apr 2012 21:09

Re: Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts

On Mon, Apr 16, 2012 at 11:27 AM, David Miller <davem <at> davemloft.net> wrote:
>
> I would like to formally propose that we change CodingStyle so that
> our comments are of the form:
>
>        /* XXX
>         * YYY
>         */
>
> So that we can save one line of vertical space.

Ugh, I personally hate it. No way will we make that the recommended
version, even if it might be an acceptable substitute in some
subsystems.

It looks unbalanced. If you want to save that vertical space, just use
C++ style comments instead, for chissake! Then you save *two* lines,
without the unbalanced thing, and just write it as

    // XXX
    // YYY

which is much preferred.

Your suggested format just sucks, and has the worst of all worlds.

                      Linus

David Miller | 16 Apr 2012 21:13

Re: Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts

From: Linus Torvalds <torvalds <at> linux-foundation.org>
Date: Mon, 16 Apr 2012 12:09:12 -0700

> It looks unbalanced. If you want to save that vertical space, just use
> C++ style comments instead, for chissake! Then you save *two* lines,
> without the unbalanced thing, and just write it as
> 
>     // XXX
>     // YYY
> 
> which is much preferred.

I'm perfectly fine with this too.

Ingo Molnar | 16 Apr 2012 22:26

Re: Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts


* David Miller <davem <at> davemloft.net> wrote:

> From: Ingo Molnar <mingo <at> kernel.org>
> Date: Mon, 16 Apr 2012 11:17:20 +0200
> 
> > 
> > This recent checkpatch commit, added in v3.4-rc1:
> > 
> >   aad4f6149831 checkpatch: add --strict tests for braces, comments and casts
> > 
> > made the default checkpatch run complain about the following 
> > perfectly fine multi-line comment block:
> > 
> > +               rdp->qlen_lazy = 0;
> > +               rdp->qlen = 0;
> > +
> > +               /*
> > +                * Adopt all callbacks.  The outgoing CPU was in no shape
> > +                * to advance them, so make them all go through a full
> > +                * grace period.
> > +                */
> > +               *receive_rdp->nxttail[RCU_NEXT_TAIL] = rdp->nxtlist;
> > 
> 
> I would like to formally propose that we change CodingStyle so that
> our comments are of the form:
> 
> 	/* XXX
> 	 * YYY

(Continue reading)

Joe Perches | 16 Apr 2012 22:34

Re: Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts

On Mon, 2012-04-16 at 22:26 +0200, Ingo Molnar wrote:
>  1) It's butt ugly and it violates every basic principle of
>     typography. Saving a line is often counter-productive, a 
>     well placed whitespace (vertical or horizontal) will often 
>     increase readability. Key is good balance.

True enough.

>     If you don't "see" it as ugly it's simply because your brain
>     is not wired up to see 3D/2D geometry as a significant
>     source of information. A significant portion (I'd 
>     guesstimate a narrow majority) of kernel hackers *does* see
>     2D/3D layout details in code and finds inconsistencies in 
>     them counterproductive.

I believe there's an empty hat somewhere that must
have produced this email.

I rather doubt anyone "sees" 3d geometries out of
a coding style.  Otherwise, cite please...

Ingo Molnar | 16 Apr 2012 22:34

Re: Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts

* Ingo Molnar <mingo <at> kernel.org> wrote:

>     If you don't "see" it as ugly it's simply because your brain
>     is not wired up to see 3D/2D geometry as a significant
>     source of information. A significant portion (I'd 
>     guesstimate a narrow majority) of kernel hackers *does* see
>     2D/3D layout details in code and finds inconsistencies in 
>     them counterproductive.
> 
>     ( It's roughly the same distinction that makes some people
>       love the typographic layout of the iPhone/iPad while 
>       others consider it unnecessary bling. )

Another example are the recent Google+ layout changes. Those who 
don't see 2D details found it an unnecessary waste of screen 
real estate.

To me and many others the new layout, while sparser, is actually 
noticeably less taxing to read, because information is 
structured in such a "geometrically obvious" way.

Thanks,

	Ingo

David Miller | 16 Apr 2012 23:00

Re: Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts

From: Ingo Molnar <mingo <at> kernel.org>
Date: Mon, 16 Apr 2012 22:34:51 +0200

> 
> * Ingo Molnar <mingo <at> kernel.org> wrote:
> 
>>     If you don't "see" it as ugly it's simply because your brain
>>     is not wired up to see 3D/2D geometry as a significant
>>     source of information. A significant portion (I'd 
>>     guesstimate a narrow majority) of kernel hackers *does* see
>>     2D/3D layout details in code and finds inconsistencies in 
>>     them counterproductive.
>> 
>>     ( It's roughly the same distinction that makes some people
>>       love the typographic layout of the iPhone/iPad while 
>>       others consider it unnecessary bling. )
> 
> Another example are the recent Google+ layout changes. Those who 
> don't see 2D details found it an unnecessary waste of screen 
> real estate.
> 
> To me and many others the new layout, while sparser, is actually 
> noticeably less taxing to read, because information is 
> structured in such a "geometrically obvious" way.

I guess us visual retards will have to find a new web site to
use then, thanks for the lesson Ingo.

Steven Rostedt | 17 Apr 2012 05:30

Re: Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts

On Mon, Apr 16, 2012 at 05:00:04PM -0400, David Miller wrote:
> 
> I guess us visual retards will have to find a new web site to
> use then, thanks for the lesson Ingo.

Facebook would love to welcome you back 

I must be retarded, as I still find facebook better than G+.

-- Steve

Ingo Molnar | 25 Apr 2012 10:33

Re: Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts


* David Miller <davem <at> davemloft.net> wrote:

> From: Ingo Molnar <mingo <at> kernel.org>
> Date: Mon, 16 Apr 2012 22:34:51 +0200
> 
> > 
> > * Ingo Molnar <mingo <at> kernel.org> wrote:
> > 
> >>     If you don't "see" it as ugly it's simply because your brain
> >>     is not wired up to see 3D/2D geometry as a significant
> >>     source of information. A significant portion (I'd 
> >>     guesstimate a narrow majority) of kernel hackers *does* see
> >>     2D/3D layout details in code and finds inconsistencies in 
> >>     them counterproductive.
> >> 
> >>     ( It's roughly the same distinction that makes some people
> >>       love the typographic layout of the iPhone/iPad while 
> >>       others consider it unnecessary bling. )
> > 
> > Another example are the recent Google+ layout changes. Those who 
> > don't see 2D details found it an unnecessary waste of screen 
> > real estate.
> > 
> > To me and many others the new layout, while sparser, is actually 
> > noticeably less taxing to read, because information is 
> > structured in such a "geometrically obvious" way.
> 
> I guess us visual retards will have to find a new web site to 
> use then, thanks for the lesson Ingo.

(Continue reading)

David Miller | 16 Apr 2012 22:58

Re: Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts

From: Ingo Molnar <mingo <at> kernel.org>
Date: Mon, 16 Apr 2012 22:26:54 +0200

>     If you don't "see" it as ugly it's simply because your brain
>     is not wired up to see 3D/2D geometry as a significant
>     source of information.

I'm sorry that I'm so handicapped that I cannot match your
visual capabilitites.

I hope you have a nice day too Ingo.

Ingo Molnar | 25 Apr 2012 10:42

Re: Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts

(Sorry about the late reply, was busy with other things.)

* David Miller <davem <at> davemloft.net> wrote:

> From: Ingo Molnar <mingo <at> kernel.org>
> Date: Mon, 16 Apr 2012 22:26:54 +0200
> 
> >     If you don't "see" it as ugly it's simply because your brain
> >     is not wired up to see 3D/2D geometry as a significant
> >     source of information.
> 
> I'm sorry that I'm so handicapped that I cannot match your
> visual capabilitites.

I genuinely think that it's the other way around: if what I say 
above is true then *you* have the superior vision for coding.

Geometric properties of code are a distraction really, they 
don't matter to the end result.

So in reality keeping comments balanced helps *my* disability: 
my inability to ignore functionally irrelevant patterns of 
characters...

So I am asking *you* to do me a favor: please understand my 
perspective and insert a bit more whitespace to keep code 
readable for people like me.

( The wider question is, what is the contribution weighted

(Continue reading)

Return

Return to gmane.linux.kernel.

Advertisement

Search Archive

Language

Change language

Options

Current view: Threads only / Showing only 20 lines / Not hiding cited text.
Change to All messages, whole messages, or hide cited text.

Post a message
NNTP Newsgroup
Classic Gmane web interface
XML

XML

RSS Feed
List Information

About Gmane