[RFD][checkpatch] warnings on space in front of labels

[RFD][checkpatch] warnings on space in front of labels

[Steven Rostedt]

Hi Andy,

There's a new warning that I've seen lately. It is about complaining
about spaces starting on a new line.

WARNING: please, no space for starting a line, 
				excluding comments
#90: FILE: trace-read.c:612:
+ again:$


Comments are currently the exception, but I would also like to add
labels too.

I always do labels as:

	[...]
	goto out;
	[...]
 out:
^
space


I do this because of patches. The patches that we use show the function
that the change is in. This is extremely helpful. But it fails when
there's a label in the function that starts on the first column, because
the patch will reference the label instead of the function.  If that
label is used in several functions, it makes it difficult to figure out
exactly what the patch is changing, and thus, it makes it harder to
review.

Doing a: git grep '^again:' to find such examples I found an example in
kernel/sched_clock.c

static u64 sched_clock_remote(struct sched_clock_data *scd)
{
	struct sched_clock_data *my_scd = this_scd();
	u64 this_clock, remote_clock;
	u64 *ptr, old_val, val;

	sched_clock_local(my_scd);
again:
	this_clock = my_scd->clock;
	remote_clock = scd->clock;

Doing a git blame, I see there was a change after this label. Doing a
git show on that commit I have:

git show 152f9d0710a62708710161bce1b29fa8292c8c11

which has:

--- a/kernel/sched_clock.c
+++ b/kernel/sched_clock.c
@@ -127,7 +127,7 @@ again:
        clock = wrap_max(clock, min_clock);
        clock = wrap_min(clock, max_clock);
 
-       if (cmpxchg(&scd->clock, old_clock, clock) != old_clock)
+       if (cmpxchg64(&scd->clock, old_clock, clock) != old_clock)
                goto again;
 
        return clock;
@@ -163,7 +163,7 @@ again:
                val = remote_clock;
        }
 
-       if (cmpxchg(ptr, old_val, val) != old_val)
+       if (cmpxchg64(ptr, old_val, val) != old_val)
                goto again;
 
        return val;



Notice the @@ again: in the header of the sections. This bothers me
because it makes it harder to review. If the 'again:' labels had a space
in front, the patch would have looked like this:

--- a/kernel/sched_clock.c
+++ b/kernel/sched_clock.c
@@ -127,7 +127,7 @@ static u64 sched_clock_local(struct sched_clock_data *scd)
        clock = wrap_max(clock, min_clock);
        clock = wrap_min(clock, max_clock);
 
-       if (cmpxchg(&scd->clock, old_clock, clock) != old_clock)
+       if (cmpxchg64(&scd->clock, old_clock, clock) != old_clock)
                goto again;
 
        return clock;
@@ -163,7 +163,7 @@ static u64 sched_clock_remote(struct sched_clock_data *scd)
                val = remote_clock;
        }
 
-       if (cmpxchg(ptr, old_val, val) != old_val)
+       if (cmpxchg64(ptr, old_val, val) != old_val)
                goto again;
 
        return val;


In fact, the first version looked like it changed only one function.
With the added space, it shows that it changed two functions.

I really prefer the space in front of the label. In fact, I think it
should be the default.

But could we at least remove the warning for spaces in front of labels?

What do others think?

Thanks,

-- Steve

Re: [RFD][checkpatch] warnings on space in front of labels

[Andy Whitcroft]

On Thu, Sep 02, 2010 at 10:18:18AM -0400, Steven Rostedt wrote:
> Hi Andy,
> 
> There's a new warning that I've seen lately. It is about complaining
> about spaces starting on a new line.
> 
> WARNING: please, no space for starting a line, 
> 				excluding comments
> #90: FILE: trace-read.c:612:
> + again:$
> 
> 
> Comments are currently the exception, but I would also like to add
> labels too.
> 
> I always do labels as:
> 
> 	[...]
> 	goto out;
> 	[...]
>  out:
> ^
> space
> 
> 
> I do this because of patches. The patches that we use show the function
> that the change is in. This is extremely helpful. But it fails when
> there's a label in the function that starts on the first column, because
> the patch will reference the label instead of the function.  If that
> label is used in several functions, it makes it difficult to figure out
> exactly what the patch is changing, and thus, it makes it harder to
> review.
> 
> Doing a: git grep '^again:' to find such examples I found an example in
> kernel/sched_clock.c
> 
> static u64 sched_clock_remote(struct sched_clock_data *scd)
> {
> 	struct sched_clock_data *my_scd = this_scd();
> 	u64 this_clock, remote_clock;
> 	u64 *ptr, old_val, val;
> 
> 	sched_clock_local(my_scd);
> again:
> 	this_clock = my_scd->clock;
> 	remote_clock = scd->clock;
> 
> Doing a git blame, I see there was a change after this label. Doing a
> git show on that commit I have:
> 
> git show 152f9d0710a62708710161bce1b29fa8292c8c11
> 
> which has:
> 
> --- a/kernel/sched_clock.c
> +++ b/kernel/sched_clock.c
> @@ -127,7 +127,7 @@ again:
>         clock = wrap_max(clock, min_clock);
>         clock = wrap_min(clock, max_clock);
>  
> -       if (cmpxchg(&scd->clock, old_clock, clock) != old_clock)
> +       if (cmpxchg64(&scd->clock, old_clock, clock) != old_clock)
>                 goto again;
>  
>         return clock;
> @@ -163,7 +163,7 @@ again:
>                 val = remote_clock;
>         }
>  
> -       if (cmpxchg(ptr, old_val, val) != old_val)
> +       if (cmpxchg64(ptr, old_val, val) != old_val)
>                 goto again;
>  
>         return val;
> 
> 
> 
> Notice the @@ again: in the header of the sections. This bothers me
> because it makes it harder to review. If the 'again:' labels had a space
> in front, the patch would have looked like this:
> 
> --- a/kernel/sched_clock.c
> +++ b/kernel/sched_clock.c
> @@ -127,7 +127,7 @@ static u64 sched_clock_local(struct sched_clock_data *scd)
>         clock = wrap_max(clock, min_clock);
>         clock = wrap_min(clock, max_clock);
>  
> -       if (cmpxchg(&scd->clock, old_clock, clock) != old_clock)
> +       if (cmpxchg64(&scd->clock, old_clock, clock) != old_clock)
>                 goto again;
>  
>         return clock;
> @@ -163,7 +163,7 @@ static u64 sched_clock_remote(struct sched_clock_data *scd)
>                 val = remote_clock;
>         }
>  
> -       if (cmpxchg(ptr, old_val, val) != old_val)
> +       if (cmpxchg64(ptr, old_val, val) != old_val)
>                 goto again;
>  
>         return val;
> 
> 
> In fact, the first version looked like it changed only one function.
> With the added space, it shows that it changed two functions.
> 
> I really prefer the space in front of the label. In fact, I think it
> should be the default.
> 
> But could we at least remove the warning for spaces in front of labels?
> 
> What do others think?

As I recall they are specified to have at least one space for exactly
this reason.  That change Andrew pulled in did have a few bugs and that
was one of them.  I believe that it should be fixed by the version in
-mm and the one I posted a link to earlier.  It does seem to accept a
naive test.  If its not working for you could you zap me the file with
the example.

-apw

Re: [RFD][checkpatch] warnings on space in front of labels

[Peter Zijlstra]

On Thu, 2010-09-02 at 10:18 -0400, Steven Rostedt wrote:
> 
> There's a new warning that I've seen lately. It is about complaining
> about spaces starting on a new line.
> 
> WARNING: please, no space for starting a line, 
>                                 excluding comments
> #90: FILE: trace-read.c:612:
> + again:$
> 
> 
> Comments are currently the exception, but I would also like to add
> labels too. 

I really hate that /^ label/ stuff and I'm removing them everywhere I
can.


> I really prefer the space in front of the label. In fact, I think it
> should be the default.
> 
> But could we at least remove the warning for spaces in front of labels?

No, kill those spaces with prejudice.

> What do others think?

I'd prefer to run a script over the kernel and remove all those silly spaces.

Re: [RFD][checkpatch] warnings on space in front of labels

[Steven Rostedt]

On Thu, 2010-09-02 at 16:31 +0200, Peter Zijlstra wrote:

> > What do others think?
> 
> I'd prefer to run a script over the kernel and remove all those silly spaces.

Until we get a diff option of --ignore-labels as a standard distro diff,
I'd NAK that patch ;-)

-- Steve

Re: [RFD][checkpatch] warnings on space in front of labels

[Peter Zijlstra]

On Thu, 2010-09-02 at 10:43 -0400, Steven Rostedt wrote:
> On Thu, 2010-09-02 at 16:31 +0200, Peter Zijlstra wrote:
> 
> > > What do others think?
> > 
> > I'd prefer to run a script over the kernel and remove all those silly spaces.
> 
> Until we get a diff option of --ignore-labels as a standard distro diff,
> I'd NAK that patch ;-)

For kicks, try:
  export QUILT_DIFF_OPTS="-F ^[[:alpha:]$_].*[^:]$"

and try again.