[Xen-devel] [PATCH 0/2] ./CODING_STYLE adjustments

[Xen-devel] [PATCH 0/2] ./CODING_STYLE adjustments

[Jan Beulich]

Patch 1 was sent before, iirc without ever having heard back anything.
Patch 2 is a result of a recent observation of Tamas.

1: explicitly call out label indentation
2: list further brace placement exceptions

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

[Xen-devel] [PATCH 1/2 RESEND] 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

[Xen-devel] [PATCH 2/2] CODING_STYLE: list further brace placement exceptions

[Jan Beulich]

For easy spotting of struct/union/enum definitions we already commonly
place the opening braces on the initial line of such a definition.

We also often don't place the opening brace of an initializer on a
separate line.

And finally for compound literals placing the braces on separate lines
often makes the code more difficult to read, so it should (and in
practice does) typically go on the same line as well.  The placement of
the closing brace often depends on how large such a compound literal is.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: We may want to make explicit that for initializers both forms are
      fine.

--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -64,8 +64,13 @@ Bracing
  -------
  
  Braces ('{' and '}') are usually placed on a line of their own, except
-for the do/while loop.  This is unlike the Linux coding style and
-unlike K&R.  do/while loops are an exception. e.g.:
+for
+- the do/while loop
+- the opening brace in definitions of enum, struct, and union
+- the opening brace in initializers
+- compound literals
+This is unlike the Linux coding style and unlike K&R.  do/while loops
+are one exception. e.g.:
  
  if ( condition )
  {

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

Re: [Xen-devel] [PATCH 1/2 RESEND] CODING_STYLE: explicitly call out label indentation

[Tamas K Lengyel]

On Fri, Jul 19, 2019 at 3:19 AM Jan Beulich <JBeulich@suse.com> 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>

This style requirement wouldn't really work with astyle as-is.

Tamas

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

Re: [Xen-devel] [PATCH 1/2 RESEND] CODING_STYLE: explicitly call out label indentation

[Jan Beulich]

On 19.07.2019 15:18, Tamas K Lengyel wrote:
> On Fri, Jul 19, 2019 at 3:19 AM Jan Beulich <JBeulich@suse.com> 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>
> 
> This style requirement wouldn't really work with astyle as-is.

Personally I view proper "diff -p" context in patches as quite
a bit more important than automatic style checking. But perhaps
that's just because I do quite a lot of patch review ...

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

Re: [Xen-devel] [PATCH 1/2 RESEND] CODING_STYLE: explicitly call out label indentation

[Tamas K Lengyel]

On Fri, Jul 19, 2019 at 7:22 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> On 19.07.2019 15:18, Tamas K Lengyel wrote:
> > On Fri, Jul 19, 2019 at 3:19 AM Jan Beulich <JBeulich@suse.com> 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>
> >
> > This style requirement wouldn't really work with astyle as-is.
>
> Personally I view proper "diff -p" context in patches as quite
> a bit more important than automatic style checking. But perhaps
> that's just because I do quite a lot of patch review ...

Which is a valid point. I don't really care which style option it
really is, if it helps you, I'm fine with it. But as a maintainer who
does this on a volunteer basis when I have a 5-minute window, I'm not
going to spend my time looking for style issues. So in the ongoing
vm_event series that's under review where you explicitly called out
that the vm_event maintainers have to review and point out all style
issues, that's a no-go from my side. So either we have automatic style
checks or I'm just going to Ack patches with style issues.

Tamas

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

Re: [Xen-devel] [PATCH 2/2] CODING_STYLE: list further brace placement exceptions

[Volodymyr Babchuk]

Hi Jan,

Jan Beulich writes:

> For easy spotting of struct/union/enum definitions we already commonly
> place the opening braces on the initial line of such a definition.
>
> We also often don't place the opening brace of an initializer on a
> separate line.
>
> And finally for compound literals placing the braces on separate lines
> often makes the code more difficult to read, so it should (and in
> practice does) typically go on the same line as well.  The placement of
> the closing brace often depends on how large such a compound literal is.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: We may want to make explicit that for initializers both forms are
>       fine.
>
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -64,8 +64,13 @@ Bracing
>   -------
>
>   Braces ('{' and '}') are usually placed on a line of their own, except
> -for the do/while loop.  This is unlike the Linux coding style and
> -unlike K&R.  do/while loops are an exception. e.g.:
> +for
> +- the do/while loop
> +- the opening brace in definitions of enum, struct, and union
> +- the opening brace in initializers
> +- compound literals
Looks like this leaves us only with "if/else", "for", "switch" and
various forms of "for_each_*". So maybe it is worth to rewrite this
in the opposite manner? Like this:

Braces ('{' and '}') are usually placed on the same line, except the
following cases:

 - if, else, for, switch statements
 - for_each_* iterators like for_each_vcpu

> +This is unlike the Linux coding style and unlike K&R.  do/while loops
There is extra space before "do/while".

> +are one exception. e.g.:
>
>   if ( condition )
>   {
>


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

Re: [Xen-devel] [PATCH 2/2] CODING_STYLE: list further brace placement exceptions

[Jan Beulich]

On 19.07.2019 18:48, Volodymyr Babchuk wrote:
> Jan Beulich writes:
>> --- a/CODING_STYLE
>> +++ b/CODING_STYLE
>> @@ -64,8 +64,13 @@ Bracing
>>    -------
>>
>>    Braces ('{' and '}') are usually placed on a line of their own, except
>> -for the do/while loop.  This is unlike the Linux coding style and
>> -unlike K&R.  do/while loops are an exception. e.g.:
>> +for
>> +- the do/while loop
>> +- the opening brace in definitions of enum, struct, and union
>> +- the opening brace in initializers
>> +- compound literals
> Looks like this leaves us only with "if/else", "for", "switch" and
> various forms of "for_each_*". So maybe it is worth to rewrite this
> in the opposite manner? Like this:

That's an option, but I'm not sure I'd want to go that route. Note
also how you don't mention e.g. "asm" and "return".

> Braces ('{' and '}') are usually placed on the same line, except the
> following cases:
> 
>   - if, else, for, switch statements
>   - for_each_* iterators like for_each_vcpu

For the latter I think we want to stick to not mandating style
either way: There's already a fair amount of either of the two
legitimate forms. It's really a matter of personal taste whether
to consider these pseudo-keywords.

>> +This is unlike the Linux coding style and unlike K&R.  do/while loops
> There is extra space before "do/while".

No. In such documents it is quite common to have double blanks
after a full stop. See e.g. the original text, which had two
instances of this.

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

Re: [PATCH 1/2 RESEND] CODING_STYLE: explicitly call out label indentation

[Stefano Stabellini]

On Fri, 19 Jul 2019, 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>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


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

Re: [PATCH 2/2] CODING_STYLE: list further brace placement exceptions

[Stefano Stabellini]

On Fri, 19 Jul 2019, Jan Beulich wrote:
> For easy spotting of struct/union/enum definitions we already commonly
> place the opening braces on the initial line of such a definition.
> 
> We also often don't place the opening brace of an initializer on a
> separate line.
> 
> And finally for compound literals placing the braces on separate lines
> often makes the code more difficult to read, so it should (and in
> practice does) typically go on the same line as well.  The placement of
> the closing brace often depends on how large such a compound literal is.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> TBD: We may want to make explicit that for initializers both forms are
>       fine.
> 
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -64,8 +64,13 @@ Bracing
>   -------
>   
>   Braces ('{' and '}') are usually placed on a line of their own, except
> -for the do/while loop.  This is unlike the Linux coding style and
> -unlike K&R.  do/while loops are an exception. e.g.:
> +for
> +- the do/while loop
> +- the opening brace in definitions of enum, struct, and union
> +- the opening brace in initializers
> +- compound literals
> +This is unlike the Linux coding style and unlike K&R.  do/while loops
> +are one exception. e.g.:
>   
>   if ( condition )
>   {
> 
>