[PATCH] CODING_STYLE: explicitly call out label indentation

[PATCH] CODING_STYLE: explicitly call out label indentation

[Jan Beulich]

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.
+
 White space
 -----------
 



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

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

[Wei Liu]

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:

*** kernel.c.orig       2018-11-27 15:15:20.841296089 +0000
--- kernel.c    2018-11-27 15:21:01.456360128 +0000
*************** static int assign_integer_param(const st
*** 48,54 ****
      default:
          BUG();
      }
!
      return 0;
  }

--- 48,54 ----
      default:
          BUG();
      }
! label:
      return 0;
  }


They look the same to me. 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.

Wei.

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

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

[Jan Beulich]

>>> 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

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

[Ian Jackson]

Since there was some confusion about what we are talking about, see
below.  Obviously the diff output in my `1' test cases is
prefereable.   Note that `git diff' does the same thing as diff -p
(and it doesn't even need a -p option to do it).

I also observe that by default, emacs wants to indent the label by 1
character - even though usually it likes to align labels to the LHS of
the enclosing block.  Presumably for this reason.

Ian.

$ cat t.c
function(){
    code;
out:
    context1;
    context2;
    context3;
    context4;
    will be edited;
    context5;
    context6;
    context7;
}
$ perl -pe 's/will be/was/' <t.c >u.c
$ perl -pe 's/out:/ $&/' <t.c >t1.c
$ perl -pe 's/will be/was/' <t1.c >u1.c
$ diff -up t.c u.c
--- t.c 2018-11-27 16:14:33.614941668 +0000
+++ u.c 2018-11-27 16:15:39.020244882 +0000
@@ -5,7 +5,7 @@ out:
     context2;
     context3;
     context4;
-    will be edited;
+    was edited;
     context5;
     context6;
     context7;
$ diff -up t1.c u1.c
--- t1.c        2018-11-27 16:15:52.424512251 +0000
+++ u1.c        2018-11-27 16:15:57.064604805 +0000
@@ -5,7 +5,7 @@ function(){
     context2;
     context3;
     context4;
-    will be edited;
+    was edited;
     context5;
     context6;
     context7;
$ 

diff --git a/t.c b/t.c
index e0a0315..d09f948 100644
--- a/t.c
+++ b/t.c
@@ -5,7 +5,7 @@ out:
     context2;
     context3;
     context4;
-    will be edited;
+    was edited;
     context5;
     context6;
     context7;
diff --git a/t1.c b/t1.c
index d7ef2c4..bc8bba1 100644
--- a/t1.c
+++ b/t1.c
@@ -5,7 +5,7 @@ function(){
     context2;
     context3;
     context4;
-    will be edited;
+    was edited;
     context5;
     context6;
     context7;

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

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

[Andrew Cooper]

On 27/11/2018 16:22, Ian Jackson wrote:
> Since there was some confusion about what we are talking about, see
> below.  Obviously the diff output in my `1' test cases is
> prefereable.   Note that `git diff' does the same thing as diff -p
> (and it doesn't even need a -p option to do it).

After some investigation, git does the correct thing when you ask to
treat c files as c files.

andrewcoop@andrewcoop:~$ cat .config/git/attributes
*.[hc] diff=cpp

This has the additional side effect of making `git diff --color-words`
and friends far more legible and nice to use.  Its a shame this isn't
the default.

> I also observe that by default, emacs wants to indent the label by 1
> character - even though usually it likes to align labels to the LHS of
> the enclosing block.  Presumably for this reason.

And after some investigation, emacs does the wrong thing in the Xen tree
because we explicitly ask for BSD style in the local block.

We should make a choice, then fix our automatic tooling to not force
code to be non-compliant.

~Andrew

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

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

[George Dunlap]

> On Nov 27, 2018, at 4:48 PM, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
> On 27/11/2018 16:22, Ian Jackson wrote:
>> Since there was some confusion about what we are talking about, see
>> below.  Obviously the diff output in my `1' test cases is
>> prefereable.   Note that `git diff' does the same thing as diff -p
>> (and it doesn't even need a -p option to do it).
> 
> After some investigation, git does the correct thing when you ask to
> treat c files as c files.
> 
> andrewcoop@andrewcoop:~$ cat .config/git/attributes
> *.[hc] diff=cpp
> 
> This has the additional side effect of making `git diff --color-words`
> and friends far more legible and nice to use.  Its a shame this isn't
> the default.
> 
>> I also observe that by default, emacs wants to indent the label by 1
>> character - even though usually it likes to align labels to the LHS of
>> the enclosing block.  Presumably for this reason.
> 
> And after some investigation, emacs does the wrong thing in the Xen tree
> because we explicitly ask for BSD style in the local block.
> 
> We should make a choice, then fix our automatic tooling to not force
> code to be non-compliant.

FWIW, having labels in column 0 always looked wrong to me.  I’m happy to change the style to require at least 1 space, but I’m *not* happy to have a style enforced that contradicts what we’ve written in the emacs style blocks at the bottom of all the files.

Would it be OK if we 
* Checked in this patch, but
* Weren’t picky about enforcing it yet
* Looked into an efficient way to get a suitable style update?

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

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

[Jan Beulich]

>>> On 27.11.18 at 18:09, <George.Dunlap@citrix.com> wrote:
>> On Nov 27, 2018, at 4:48 PM, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
>> On 27/11/2018 16:22, Ian Jackson wrote:
>>> Since there was some confusion about what we are talking about, see
>>> below.  Obviously the diff output in my `1' test cases is
>>> prefereable.   Note that `git diff' does the same thing as diff -p
>>> (and it doesn't even need a -p option to do it).
>> 
>> After some investigation, git does the correct thing when you ask to
>> treat c files as c files.
>> 
>> andrewcoop@andrewcoop:~$ cat .config/git/attributes
>> *.[hc] diff=cpp
>> 
>> This has the additional side effect of making `git diff --color-words`
>> and friends far more legible and nice to use.  Its a shame this isn't
>> the default.
>> 
>>> I also observe that by default, emacs wants to indent the label by 1
>>> character - even though usually it likes to align labels to the LHS of
>>> the enclosing block.  Presumably for this reason.
>> 
>> And after some investigation, emacs does the wrong thing in the Xen tree
>> because we explicitly ask for BSD style in the local block.
>> 
>> We should make a choice, then fix our automatic tooling to not force
>> code to be non-compliant.
> 
> FWIW, having labels in column 0 always looked wrong to me.  I’m happy to 
> change the style to require at least 1 space, but I’m *not* happy to have a 
> style enforced that contradicts what we’ve written in the emacs style blocks 
> at the bottom of all the files.
> 
> Would it be OK if we 
> * Checked in this patch, but
> * Weren’t picky about enforcing it yet

FWIW, I've been picky about this for the last so many years.

Jan

> * Looked into an efficient way to get a suitable style update?
> 
>  -George




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