Re: [PATCH] CODING_STYLE: explicitly call out label indentation

["Jan Beulich" <JBeulich@suse.com>]

>>> On 27.11.18 at 16:23, <wei.liu2@citrix.com> wrote:
> On Mon, Nov 26, 2018 at 02:04:05AM -0700, Jan Beulich wrote:
>> Since the behavior of "diff -p" to use an unindented label as context
>> identifier often makes it harder to review patches, make explicit the
>> requirement for labels to be indented.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> --- a/CODING_STYLE
>> +++ b/CODING_STYLE
>> @@ -31,6 +31,10 @@ void fun(void)
>>      }
>>  }
>>  
>> +Due to the behavior of GNU diffutils "diff -p", labels should be
>> +indented by at least one blank.  Non-case labels inside switch() bodies
>> +are preferred to be indented the same as the block's case labels.
>> +
> 
> Sorry, I don't follow this rationale.
> 
> I actually tried `diff -p` with and without indenting the label. Here is
> the result.
> 
> With:
> 
> *** kernel.c.orig       2018-11-27 15:15:20.841296089 +0000
> --- kernel.c    2018-11-27 15:20:23.192022064 +0000
> *************** static int assign_integer_param(const st
> *** 48,54 ****
>       default:
>           BUG();
>       }
> !
>       return 0;
>   }
> 
> --- 48,54 ----
>       default:
>           BUG();
>       }
> !  label:
>       return 0;
>   }
> 
> 
> Without:
> [...]
> They look the same to me.

Well, that's because you used a change as example where you're
_adding_ a label, whereas the issue is with other additions which
_follow_ an earlier label.

> And frankly having an extra space in front make Xen
> rather too unique. That's an issue for new comers and writing automated tool
> to check patch.

If other projects don't care about this and are happy to review
patches to files with, say, many unindented "retry" labels (in
different functions), then that's their issue. _I_ dislike reviewing
patches where I can't easily identify which function is getting
changed. And based on that I also dislike submitting patches
where this isn't easily possible.

I think I've also come to a conclusion as to why they may prefer
to leave diff behavior as it is: For assembler files the behavior is
actually useful.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel