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

[Andy Whitcroft <apw@canonical.com>]

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