[Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

Using astyle (http://astyle.sourceforge.net) can greatly reduce the overhead of
manually checking and applying style-fixes to source-code. The included
.astylerc is the closest approximation of the established Xen style (including
styles not formally spelled out by CODING_STYLE but commonly requested).

Checking the comment styles are not included in the automation.

Incorporating Xen's exception to the do-while style is only partially possible,
thus a change is proposed to the CODING_STYLE of moving the brace from "do {"
to the next line.

Most of Xen's code-base is non-conforming at the moment: 289 files pass
unchanged, 876 have some style issues.

Ideally we can slowly migrate the entire code-base to be conforming, thus
eliminating the need of discussing and enforcing style issues manually on the
mailinglist.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
---
 .astylerc    | 14 ++++++++++++++
 CODING_STYLE | 18 +++++++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)
 create mode 100644 .astylerc

diff --git a/.astylerc b/.astylerc
new file mode 100644
index 0000000000..bbd1d55ddd
--- /dev/null
+++ b/.astylerc
@@ -0,0 +1,14 @@
+style=bsd
+suffix=none
+align-pointer=name
+align-reference=name
+indent=spaces=4
+max-code-length=80
+min-conditional-indent=0
+attach-closing-while
+remove-braces
+indent-switches
+break-one-line-headers
+keep-one-line-blocks
+pad-comma
+pad-header
diff --git a/CODING_STYLE b/CODING_STYLE
index 6cc5b774cf..0b37f7ae4d 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -60,8 +60,8 @@ 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 while-part of do/while loops.  This is unlike the Linux coding style
+and unlike K&R.  do/while loops are an exception. e.g.:
 
 if ( condition )
 {
@@ -77,7 +77,8 @@ while ( condition )
     /* Do stuff. */
 }
 
-do {
+do
+{
     /* Do stuff. */
 } while ( condition );
 
@@ -120,3 +121,14 @@ the end of files.  It should be:
  * indent-tabs-mode: nil
  * End:
  */
+
+Automated style formatting using astyle
+---------------------------------------
+
+The .astylerc included in the Xen tree incorporates most of Xen's
+style requirements, except the formatting of comments.
+
+The steps to automatically format a file are:
+
+export ARTISTIC_STYLE_OPTIONS=".astylerc"
+astyle <source or header file>
-- 
2.20.1


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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

Hi Tamas,

Adding Lars, Artem and Iurii. Iurii has been working on a version for 
clang-format recently.

On 7/18/19 3:43 PM, Tamas K Lengyel wrote:
> Using astyle (http://astyle.sourceforge.net) can greatly reduce the overhead of
> manually checking and applying style-fixes to source-code. The included
> .astylerc is the closest approximation of the established Xen style (including
> styles not formally spelled out by CODING_STYLE but commonly requested).
> 
> Checking the comment styles are not included in the automation.
> 
> Incorporating Xen's exception to the do-while style is only partially possible,
> thus a change is proposed to the CODING_STYLE of moving the brace from "do {"
> to the next line.
> 
> Most of Xen's code-base is non-conforming at the moment: 289 files pass
> unchanged, 876 have some style issues.
> 
> Ideally we can slowly migrate the entire code-base to be conforming, thus
> eliminating the need of discussing and enforcing style issues manually on the
> mailinglist.

I quite like the idea of an automatic coding style checker. However, it 
is a bit concerning that not even a 1/3 of the files are able to pass 
the coding style you suggest. Could you explain whether this is because 
the files does not already follow Xen coding style or is it just the 
difference with astyle?

What are the main style issues?

> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> ---
>   .astylerc    | 14 ++++++++++++++
>   CODING_STYLE | 18 +++++++++++++++---
>   2 files changed, 29 insertions(+), 3 deletions(-)
>   create mode 100644 .astylerc
> 
> diff --git a/.astylerc b/.astylerc
> new file mode 100644
> index 0000000000..bbd1d55ddd
> --- /dev/null
> +++ b/.astylerc
> @@ -0,0 +1,14 @@
> +style=bsd
> +suffix=none
> +align-pointer=name
> +align-reference=name
> +indent=spaces=4
> +max-code-length=80
> +min-conditional-indent=0
> +attach-closing-while
> +remove-braces
> +indent-switches
> +break-one-line-headers
> +keep-one-line-blocks
> +pad-comma
> +pad-header
> diff --git a/CODING_STYLE b/CODING_STYLE
> index 6cc5b774cf..0b37f7ae4d 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -60,8 +60,8 @@ 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 while-part of do/while loops.  This is unlike the Linux coding style
> +and unlike K&R.  do/while loops are an exception. e.g.:
>   
>   if ( condition )
>   {
> @@ -77,7 +77,8 @@ while ( condition )
>       /* Do stuff. */
>   }
>   
> -do {
> +do
> +{
>       /* Do stuff. */
>   } while ( condition );
>   
> @@ -120,3 +121,14 @@ the end of files.  It should be:
>    * indent-tabs-mode: nil
>    * End:
>    */
> +
> +Automated style formatting using astyle
> +---------------------------------------
> +
> +The .astylerc included in the Xen tree incorporates most of Xen's
> +style requirements, except the formatting of comments.
> +
> +The steps to automatically format a file are:
> +
> +export ARTISTIC_STYLE_OPTIONS=".astylerc"
> +astyle <source or header file>

I think you want to provide easy way for the user to install/compile it. 
So there are an higher chance for them to use it.

Long-term we probably want to get this hooked to the CI loop.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

On Thu, Jul 18, 2019 at 9:02 AM Julien Grall <julien.grall@arm.com> wrote:
>
> Hi Tamas,
>
> Adding Lars, Artem and Iurii. Iurii has been working on a version for
> clang-format recently.
>
> On 7/18/19 3:43 PM, Tamas K Lengyel wrote:
> > Using astyle (http://astyle.sourceforge.net) can greatly reduce the overhead of
> > manually checking and applying style-fixes to source-code. The included
> > .astylerc is the closest approximation of the established Xen style (including
> > styles not formally spelled out by CODING_STYLE but commonly requested).
> >
> > Checking the comment styles are not included in the automation.
> >
> > Incorporating Xen's exception to the do-while style is only partially possible,
> > thus a change is proposed to the CODING_STYLE of moving the brace from "do {"
> > to the next line.
> >
> > Most of Xen's code-base is non-conforming at the moment: 289 files pass
> > unchanged, 876 have some style issues.
> >
> > Ideally we can slowly migrate the entire code-base to be conforming, thus
> > eliminating the need of discussing and enforcing style issues manually on the
> > mailinglist.
>
> I quite like the idea of an automatic coding style checker. However, it
> is a bit concerning that not even a 1/3 of the files are able to pass
> the coding style you suggest. Could you explain whether this is because
> the files does not already follow Xen coding style or is it just the
> difference with astyle?
>
> What are the main style issues?

Looks like a lot of files that don't follow the Xen coding style
as-is. Alignment issues seem to me to be the most common errors. See
the full diff here:

https://gist.github.com/tklengyel/c5cac14a0d57f119dd7747a1be6fb260

We can perhaps tune some aspects of it we disagree with the astyle
generated style and try to override it. I did my best to make it
conform to the existing Xen style but certainly there could be other
tweaks made to reduce the churn.

> >
> > Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> > ---
> >   .astylerc    | 14 ++++++++++++++
> >   CODING_STYLE | 18 +++++++++++++++---
> >   2 files changed, 29 insertions(+), 3 deletions(-)
> >   create mode 100644 .astylerc
> >
> > diff --git a/.astylerc b/.astylerc
> > new file mode 100644
> > index 0000000000..bbd1d55ddd
> > --- /dev/null
> > +++ b/.astylerc
> > @@ -0,0 +1,14 @@
> > +style=bsd
> > +suffix=none
> > +align-pointer=name
> > +align-reference=name
> > +indent=spaces=4
> > +max-code-length=80
> > +min-conditional-indent=0
> > +attach-closing-while
> > +remove-braces
> > +indent-switches
> > +break-one-line-headers
> > +keep-one-line-blocks
> > +pad-comma
> > +pad-header
> > diff --git a/CODING_STYLE b/CODING_STYLE
> > index 6cc5b774cf..0b37f7ae4d 100644
> > --- a/CODING_STYLE
> > +++ b/CODING_STYLE
> > @@ -60,8 +60,8 @@ 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 while-part of do/while loops.  This is unlike the Linux coding style
> > +and unlike K&R.  do/while loops are an exception. e.g.:
> >
> >   if ( condition )
> >   {
> > @@ -77,7 +77,8 @@ while ( condition )
> >       /* Do stuff. */
> >   }
> >
> > -do {
> > +do
> > +{
> >       /* Do stuff. */
> >   } while ( condition );
> >
> > @@ -120,3 +121,14 @@ the end of files.  It should be:
> >    * indent-tabs-mode: nil
> >    * End:
> >    */
> > +
> > +Automated style formatting using astyle
> > +---------------------------------------
> > +
> > +The .astylerc included in the Xen tree incorporates most of Xen's
> > +style requirements, except the formatting of comments.
> > +
> > +The steps to automatically format a file are:
> > +
> > +export ARTISTIC_STYLE_OPTIONS=".astylerc"
> > +astyle <source or header file>
>
> I think you want to provide easy way for the user to install/compile it.
> So there are an higher chance for them to use it.

It is generally included in all major distributions, I just do apt-get
install astyle on debian/ubuntu.

>
> Long-term we probably want to get this hooked to the CI loop.

Indeed. I already do that in my project's CI using Travis and it has
been awesome (https://github.com/tklengyel/drakvuf/blob/master/.travis.yml#L28).

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

Hi Tamas,

On 7/18/19 4:14 PM, Tamas K Lengyel wrote:
> On Thu, Jul 18, 2019 at 9:02 AM Julien Grall <julien.grall@arm.com> wrote:
>>
>> Hi Tamas,
>>
>> Adding Lars, Artem and Iurii. Iurii has been working on a version for
>> clang-format recently.
>>
>> On 7/18/19 3:43 PM, Tamas K Lengyel wrote:
>>> Using astyle (http://astyle.sourceforge.net) can greatly reduce the overhead of
>>> manually checking and applying style-fixes to source-code. The included
>>> .astylerc is the closest approximation of the established Xen style (including
>>> styles not formally spelled out by CODING_STYLE but commonly requested).
>>>
>>> Checking the comment styles are not included in the automation.
>>>
>>> Incorporating Xen's exception to the do-while style is only partially possible,
>>> thus a change is proposed to the CODING_STYLE of moving the brace from "do {"
>>> to the next line.
>>>
>>> Most of Xen's code-base is non-conforming at the moment: 289 files pass
>>> unchanged, 876 have some style issue
>>>
>>> Ideally we can slowly migrate the entire code-base to be conforming, thus
>>> eliminating the need of discussing and enforcing style issues manually on the
>>> mailinglist.
>>
>> I quite like the idea of an automatic coding style checker. However, it
>> is a bit concerning that not even a 1/3 of the files are able to pass
>> the coding style you suggest. Could you explain whether this is because
>> the files does not already follow Xen coding style or is it just the
>> difference with astyle?
>>
>> What are the main style issues?
> 
> Looks like a lot of files that don't follow the Xen coding style
> as-is. Alignment issues seem to me to be the most common errors. See
> the full diff here:
> 
> https://gist.github.com/tklengyel/c5cac14a0d57f119dd7747a1be6fb260
> 
> We can perhaps tune some aspects of it we disagree with the astyle
> generated style and try to override it. I did my best to make it
> conform to the existing Xen style but certainly there could be other
> tweaks made to reduce the churn.

I think we definitely want to avoid churn as this is going to take a lot 
of time to fix all the places to the new indentation.

Going through the diff I can see major differences with the Xen Coding 
style and also what looks like inconsistencies from the tools itself:
   - Line 58: This is fairly common to indent the parameters as it is 
today. But then on line 158/272 it indents as we do today. So I am not 
sure what the expected coding style from the tools.
   - Line 67: I believe Jan request the space before label
   - Line 139: The { are commonly on the same line as struct or definition.
   - Line 276: The switch case indentation was correct from Xen PoV before
   - Line 289: Files imported from Linux should not be touch here.
   - Line 1025: The tools needs to be able to deal for_each_vcpu(...) & co.
   - Line 8735: It looks like to me the tools policy is quite 
inconsistent. In previous place it keeps it properly aligned see line 5777.

I have only looked quickly through the diff, but I think they are the 
main one that should probably be resolved.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

>    - Line 289: Files imported from Linux should not be touch here.

This is just a raw dump of what happens if I run astyle on all source
and header files. Obviously I won't attempt to upstream all these
changes you see in the gist. Respective maintainers are welcome to use
the tool if they find value in it.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

Hi,

On 18/07/2019 18:18, Tamas K Lengyel wrote:
>>     - Line 289: Files imported from Linux should not be touch here.
> 
> This is just a raw dump of what happens if I run astyle on all source
> and header files. Obviously I won't attempt to upstream all these
> changes you see in the gist. Respective maintainers are welcome to use
> the tool if they find value in it.

Maintainers usually know the style of a file. However, this is not the case of a 
users or even the CI.

I agree this is not a new problem but a list of files using Xen/Linux Coding 
Style would be helpful in the two contexts mentioned.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

>    - Line 139: The { are commonly on the same line as struct or definition.

According to CODING_STYLE that's not how it should be.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

Hi Tamas,

On 18/07/2019 18:22, Tamas K Lengyel wrote:
>>     - Line 139: The { are commonly on the same line as struct or definition.
> 
> According to CODING_STYLE that's not how it should be.

I guess you refer to the section "Bracing". If so, I think we don't follow the 
CODING_STYLE for struct or definition.

I have to admit that for this case, I always have to look at other usage in the 
code because I tend to put the { on a newline.

I was going to say that there may be some cases where it makes sense to keep { 
on the same line. For instance:

    { a, b, c, d },
    { e, f, g, h },

But it looks like astyle is allowing it (see line 24759).

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Viktor Mitin]

On Fri, Jul 19, 2019 at 11:37 AM Julien Grall <julien.grall@arm.com> wrote:
>
> Hi Tamas,
>
> On 18/07/2019 18:22, Tamas K Lengyel wrote:
> >>     - Line 139: The { are commonly on the same line as struct or definition.
> >
> > According to CODING_STYLE that's not how it should be.
>
> I guess you refer to the section "Bracing". If so, I think we don't follow the
> CODING_STYLE for struct or definition.
>
> I have to admit that for this case, I always have to look at other usage in the
> code because I tend to put the { on a newline.
>
> I was going to say that there may be some cases where it makes sense to keep {
> on the same line. For instance:
>
>     { a, b, c, d },
>     { e, f, g, h },
>

All the rules should be stated in CODING_STYLE explicitly.
In other cases, it cannot be automated with future proves why it works
this or that way.

Thanks

> But it looks like astyle is allowing it (see line 24759).
>
> Cheers,
>
> --
> Julien Grall
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

Hi Viktor,

On 26/07/2019 15:58, Viktor Mitin wrote:
> On Fri, Jul 19, 2019 at 11:37 AM Julien Grall <julien.grall@arm.com> wrote:
>>
>> Hi Tamas,
>>
>> On 18/07/2019 18:22, Tamas K Lengyel wrote:
>>>>      - Line 139: The { are commonly on the same line as struct or definition.
>>>
>>> According to CODING_STYLE that's not how it should be.
>>
>> I guess you refer to the section "Bracing". If so, I think we don't follow the
>> CODING_STYLE for struct or definition.
>>
>> I have to admit that for this case, I always have to look at other usage in the
>> code because I tend to put the { on a newline.
>>
>> I was going to say that there may be some cases where it makes sense to keep {
>> on the same line. For instance:
>>
>>      { a, b, c, d },
>>      { e, f, g, h },
>>
> 
> All the rules should be stated in CODING_STYLE explicitly.
> In other cases, it cannot be automated with future proves why it works
> this or that way.

This is not very constructive to send on multiple different threads "this should 
be explicitly be in the CODING_STYLE". Most of the people in CC are aware this 
is an issue.

But the problem here is codifying those styles. This often results in a lot of 
discussions because a lot is a matter of taste. In other there are no answers to 
the patches.

What we need is someone to drive this effort and make sure we make some progress.

Also, trying to codify all the unwritten styles is probably going to be 
difficult. Instead, I would suggest to start from an existing coding style that 
is very close to Xen (maybe BSD?).

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

On Fri, Jul 26, 2019 at 9:12 AM Julien Grall <julien.grall@arm.com> wrote:
>
> Hi Viktor,
>
> On 26/07/2019 15:58, Viktor Mitin wrote:
> > On Fri, Jul 19, 2019 at 11:37 AM Julien Grall <julien.grall@arm.com> wrote:
> >>
> >> Hi Tamas,
> >>
> >> On 18/07/2019 18:22, Tamas K Lengyel wrote:
> >>>>      - Line 139: The { are commonly on the same line as struct or definition.
> >>>
> >>> According to CODING_STYLE that's not how it should be.
> >>
> >> I guess you refer to the section "Bracing". If so, I think we don't follow the
> >> CODING_STYLE for struct or definition.
> >>
> >> I have to admit that for this case, I always have to look at other usage in the
> >> code because I tend to put the { on a newline.
> >>
> >> I was going to say that there may be some cases where it makes sense to keep {
> >> on the same line. For instance:
> >>
> >>      { a, b, c, d },
> >>      { e, f, g, h },
> >>
> >
> > All the rules should be stated in CODING_STYLE explicitly.
> > In other cases, it cannot be automated with future proves why it works
> > this or that way.
>
> This is not very constructive to send on multiple different threads "this should
> be explicitly be in the CODING_STYLE". Most of the people in CC are aware this
> is an issue.
>
> But the problem here is codifying those styles. This often results in a lot of
> discussions because a lot is a matter of taste. In other there are no answers to
> the patches.
>
> What we need is someone to drive this effort and make sure we make some progress.
>
> Also, trying to codify all the unwritten styles is probably going to be
> difficult. Instead, I would suggest to start from an existing coding style that
> is very close to Xen (maybe BSD?).

+1, going from an existing style and then adding exceptions as needed
I think is a good way to do this. At least that's what I tried with
astyle and the BSD style was pretty close.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Lars Kurth]

On 26/07/2019, 16:12, "Julien Grall" <julien.grall@arm.com> wrote:

    Hi Viktor,
    
    On 26/07/2019 15:58, Viktor Mitin wrote:
    > On Fri, Jul 19, 2019 at 11:37 AM Julien Grall <julien.grall@arm.com> wrote:
    >>
    >> Hi Tamas,
    >>
    >> On 18/07/2019 18:22, Tamas K Lengyel wrote:
    >>>>      - Line 139: The { are commonly on the same line as struct or definition.
    >>>
    >>> According to CODING_STYLE that's not how it should be.
    >>
    >> I guess you refer to the section "Bracing". If so, I think we don't follow the
    >> CODING_STYLE for struct or definition.
    >>
    >> I have to admit that for this case, I always have to look at other usage in the
    >> code because I tend to put the { on a newline.
    >>
    >> I was going to say that there may be some cases where it makes sense to keep {
    >> on the same line. For instance:
    >>
    >>      { a, b, c, d },
    >>      { e, f, g, h },
    >>
    > 
    > All the rules should be stated in CODING_STYLE explicitly.
    > In other cases, it cannot be automated with future proves why it works
    > this or that way.
    
    This is not very constructive to send on multiple different threads "this should 
    be explicitly be in the CODING_STYLE". Most of the people in CC are aware this 
    is an issue.
    
    But the problem here is codifying those styles. This often results in a lot of 
    discussions because a lot is a matter of taste. In other there are no answers to 
    the patches.
    
    What we need is someone to drive this effort and make sure we make some progress.
    
    Also, trying to codify all the unwritten styles is probably going to be 
    difficult. Instead, I would suggest to start from an existing coding style that 
    is very close to Xen (maybe BSD?).
    
OK. Maybe this is something I should drive, if there is in fact some sort of agreement that this makes sense.

I agree that codifying the styles is problematic, as it encourages bike shedding. Maybe this is something 
where we could try something vote based, aka make a list of codified rules. Have everyone vote in the usual
way of -2 ... +2 on it.

Also, we can't implement checking tools, if the styles are not documented. Checking tools should free up
reviewer time.

Lars

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Jan Beulich]

On 18.07.2019 19:22, Tamas K Lengyel wrote:
>>     - Line 139: The { are commonly on the same line as struct or definition.
> 
> According to CODING_STYLE that's not how it should be.

Having the brace on the same line there is rather helpful to easily
tell the definition point of a struct/union/enum from any of its uses.
Hence no matter whether ./CODING_STYLE doesn't explicitly cover this
fact, I think we want to stick to the (Linux inherited I think) form.

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

On Fri, Jul 19, 2019 at 3:01 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> On 18.07.2019 19:22, Tamas K Lengyel wrote:
> >>     - Line 139: The { are commonly on the same line as struct or definition.
> >
> > According to CODING_STYLE that's not how it should be.
>
> Having the brace on the same line there is rather helpful to easily
> tell the definition point of a struct/union/enum from any of its uses.
> Hence no matter whether ./CODING_STYLE doesn't explicitly cover this
> fact, I think we want to stick to the (Linux inherited I think) form.
>

Fair point, but then again we don't have to add every file to the
automated style checks, for example by adding "/* *INDENT-OFF* */" at
the top of the file. The "--exclude" command line flag can also be
used for that. Or portions of files can also be marked
not-to-be-touched with the INDENT-OFF/INDENT-ON comments.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Viktor Mitin]

Hi All,

On Thu, Jul 18, 2019 at 8:24 PM Tamas K Lengyel <tamas@tklengyel.com> wrote:
>
> >    - Line 139: The { are commonly on the same line as struct or definition.
>
> According to CODING_STYLE that's not how it should be.

All such cases should be explicitly stated in the CODING_STYLE document.

Thanks

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

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

>    - Line 276: The switch case indentation was correct from Xen PoV before

Removing "indent-switches" from the .astylerc actually leaves these
switches untouched, so that's an easy fix.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

>    - Line 58: This is fairly common to indent the parameters as it is
> today. But then on line 158/272 it indents as we do today. So I am not
> sure what the expected coding style from the tools.

This was due to the tool not allowing indentation to be further to the
right then 40 characters. Can be modified with
max-continuation-indent, so for example max-continuation-indent=78
(which is the max that would make sense in the extreme) leaves these
correct indentations untouched.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

>    - Line 8735: It looks like to me the tools policy is quite
> inconsistent. In previous place it keeps it properly aligned see line 5777.

This was also due to not allowing indentation to be more then 40
characters to the right. Overriding max-continuation-indent leaves
these be.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

>    - Line 1025: The tools needs to be able to deal for_each_vcpu(...) & co.

These can be made OK by adding braces. Other than that the only way I
found to make it not change the indentation is to add the comment "/*
*INDENT-OFF* */" before the block and "/* *INDENT-ON* */" afterwards.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

Hi Tamas,

On 18/07/2019 18:48, Tamas K Lengyel wrote:
>>     - Line 1025: The tools needs to be able to deal for_each_vcpu(...) & co.
> 
> These can be made OK by adding braces. Other than that the only way I
> found to make it not change the indentation is to add the comment "/*
> *INDENT-OFF* */" before the block and "/* *INDENT-ON* */" afterwards.

None of them looks really appealing because it means astyle will not correctly 
indent if the user does not add braces or comments.

Could astyle be easily modified to recognize foreach macros?

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

On Fri, Jul 19, 2019 at 2:43 AM Julien Grall <julien.grall@arm.com> wrote:
>
> Hi Tamas,
>
> On 18/07/2019 18:48, Tamas K Lengyel wrote:
> >>     - Line 1025: The tools needs to be able to deal for_each_vcpu(...) & co.
> >
> > These can be made OK by adding braces. Other than that the only way I
> > found to make it not change the indentation is to add the comment "/*
> > *INDENT-OFF* */" before the block and "/* *INDENT-ON* */" afterwards.
>
> None of them looks really appealing because it means astyle will not correctly
> indent if the user does not add braces or comments.
>
> Could astyle be easily modified to recognize foreach macros?

Not that I'm aware of. If you don't want to manually annotate files
with unsupported macros then just exclude those files from astyle. I
wouldn't recommend adding this to the CI for all files, only for those
that their respective maintainers have confirmed to conform to the
style and want to enforce it going forward.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

Hi Tamas,

On 19/07/2019 14:00, Tamas K Lengyel wrote:
> On Fri, Jul 19, 2019 at 2:43 AM Julien Grall <julien.grall@arm.com> wrote:
>>
>> Hi Tamas,
>>
>> On 18/07/2019 18:48, Tamas K Lengyel wrote:
>>>>      - Line 1025: The tools needs to be able to deal for_each_vcpu(...) & co.
>>>
>>> These can be made OK by adding braces. Other than that the only way I
>>> found to make it not change the indentation is to add the comment "/*
>>> *INDENT-OFF* */" before the block and "/* *INDENT-ON* */" afterwards.
>>
>> None of them looks really appealing because it means astyle will not correctly
>> indent if the user does not add braces or comments.
>>
>> Could astyle be easily modified to recognize foreach macros?
> 
> Not that I'm aware of. If you don't want to manually annotate files
> with unsupported macros then just exclude those files from astyle. I
> wouldn't recommend adding this to the CI for all files, only for those
> that their respective maintainers have confirmed to conform to the
> style and want to enforce it going forward.

So a couple use of an unsupported macros would make impossible to enforce the 
coding style. This is not a very ideal position to be in.

_if_ we are going to adopt astyle then we need to be able to enforce it on every 
Xen files long-term. If it is not possible to do it with astyle, then maybe this 
is not the right tool to use.

For instance, I know that tools such as clang-format is able to deal with 
foreach macros.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

On Fri, Jul 19, 2019 at 7:11 AM Julien Grall <julien.grall@arm.com> wrote:
>
> Hi Tamas,
>
> On 19/07/2019 14:00, Tamas K Lengyel wrote:
> > On Fri, Jul 19, 2019 at 2:43 AM Julien Grall <julien.grall@arm.com> wrote:
> >>
> >> Hi Tamas,
> >>
> >> On 18/07/2019 18:48, Tamas K Lengyel wrote:
> >>>>      - Line 1025: The tools needs to be able to deal for_each_vcpu(...) & co.
> >>>
> >>> These can be made OK by adding braces. Other than that the only way I
> >>> found to make it not change the indentation is to add the comment "/*
> >>> *INDENT-OFF* */" before the block and "/* *INDENT-ON* */" afterwards.
> >>
> >> None of them looks really appealing because it means astyle will not correctly
> >> indent if the user does not add braces or comments.
> >>
> >> Could astyle be easily modified to recognize foreach macros?
> >
> > Not that I'm aware of. If you don't want to manually annotate files
> > with unsupported macros then just exclude those files from astyle. I
> > wouldn't recommend adding this to the CI for all files, only for those
> > that their respective maintainers have confirmed to conform to the
> > style and want to enforce it going forward.
>
> So a couple use of an unsupported macros would make impossible to enforce the
> coding style. This is not a very ideal position to be in.
>
> _if_ we are going to adopt astyle then we need to be able to enforce it on every
> Xen files long-term. If it is not possible to do it with astyle, then maybe this
> is not the right tool to use.
>
> For instance, I know that tools such as clang-format is able to deal with
> foreach macros.

If there are better tools then sure, I don't really mind using
something else. I just don't have time to do the manual style check
back-and-forth anymore, so the sooner we have something in place the
better.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

Hi Tamas,

On 19/07/2019 14:14, Tamas K Lengyel wrote:
> On Fri, Jul 19, 2019 at 7:11 AM Julien Grall <julien.grall@arm.com> wrote:
>>
>> Hi Tamas,
>>
>> On 19/07/2019 14:00, Tamas K Lengyel wrote:
>>> On Fri, Jul 19, 2019 at 2:43 AM Julien Grall <julien.grall@arm.com> wrote:
>>>>
>>>> Hi Tamas,
>>>>
>>>> On 18/07/2019 18:48, Tamas K Lengyel wrote:
>>>>>>       - Line 1025: The tools needs to be able to deal for_each_vcpu(...) & co.
>>>>>
>>>>> These can be made OK by adding braces. Other than that the only way I
>>>>> found to make it not change the indentation is to add the comment "/*
>>>>> *INDENT-OFF* */" before the block and "/* *INDENT-ON* */" afterwards.
>>>>
>>>> None of them looks really appealing because it means astyle will not correctly
>>>> indent if the user does not add braces or comments.
>>>>
>>>> Could astyle be easily modified to recognize foreach macros?
>>>
>>> Not that I'm aware of. If you don't want to manually annotate files
>>> with unsupported macros then just exclude those files from astyle. I
>>> wouldn't recommend adding this to the CI for all files, only for those
>>> that their respective maintainers have confirmed to conform to the
>>> style and want to enforce it going forward.
>>
>> So a couple use of an unsupported macros would make impossible to enforce the
>> coding style. This is not a very ideal position to be in.
>>
>> _if_ we are going to adopt astyle then we need to be able to enforce it on every
>> Xen files long-term. If it is not possible to do it with astyle, then maybe this
>> is not the right tool to use.
>>
>> For instance, I know that tools such as clang-format is able to deal with
>> foreach macros.
> 
> If there are better tools then sure, I don't really mind using
> something else. I just don't have time to do the manual style check
> back-and-forth anymore, so the sooner we have something in place the
> better.

I totally agree we need a tool so the reviewer can free-up some time to focus on 
more important things. However, I think we should be careful on what we adopt here.

Similar to Andrew, I am open with modifying the coding style to help the 
automatic style check. But I am not happy to disable automatic style on part (or 
entire) of files forever.

At the moment, clang-format feels more powerful and there are people working on it.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

On 19/07/2019 14:24, Julien Grall wrote:
> Hi Tamas,
> 
> On 19/07/2019 14:14, Tamas K Lengyel wrote:
>> On Fri, Jul 19, 2019 at 7:11 AM Julien Grall <julien.grall@arm.com> wrote:
>>>
>>> Hi Tamas,
>>>
>>> On 19/07/2019 14:00, Tamas K Lengyel wrote:
>>>> On Fri, Jul 19, 2019 at 2:43 AM Julien Grall <julien.grall@arm.com> wrote:
>>>>>
>>>>> Hi Tamas,
>>>>>
>>>>> On 18/07/2019 18:48, Tamas K Lengyel wrote:
>>>>>>>       - Line 1025: The tools needs to be able to deal for_each_vcpu(...) 
>>>>>>> & co.
>>>>>>
>>>>>> These can be made OK by adding braces. Other than that the only way I
>>>>>> found to make it not change the indentation is to add the comment "/*
>>>>>> *INDENT-OFF* */" before the block and "/* *INDENT-ON* */" afterwards.
>>>>>
>>>>> None of them looks really appealing because it means astyle will not correctly
>>>>> indent if the user does not add braces or comments.
>>>>>
>>>>> Could astyle be easily modified to recognize foreach macros?
>>>>
>>>> Not that I'm aware of. If you don't want to manually annotate files
>>>> with unsupported macros then just exclude those files from astyle. I
>>>> wouldn't recommend adding this to the CI for all files, only for those
>>>> that their respective maintainers have confirmed to conform to the
>>>> style and want to enforce it going forward.
>>>
>>> So a couple use of an unsupported macros would make impossible to enforce the
>>> coding style. This is not a very ideal position to be in.
>>>
>>> _if_ we are going to adopt astyle then we need to be able to enforce it on every
>>> Xen files long-term. If it is not possible to do it with astyle, then maybe this
>>> is not the right tool to use.
>>>
>>> For instance, I know that tools such as clang-format is able to deal with
>>> foreach macros.
>>
>> If there are better tools then sure, I don't really mind using
>> something else. I just don't have time to do the manual style check
>> back-and-forth anymore, so the sooner we have something in place the
>> better.
> 
> I totally agree we need a tool so the reviewer can free-up some time to focus on 
> more important things. However, I think we should be careful on what we adopt here.
> 
> Similar to Andrew, I am open with modifying the coding style to help the 
> automatic style check. But I am not happy to disable automatic style on part (or 
> entire) of files forever.
> 
> At the moment, clang-format feels more powerful and there are people working on it.

FYI, below a link to the clang-format changes:

https://github.com/xen-troops/Xen-Clang-format/blob/devel/clang-format.patch

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Rich Persaud]

On Jul 19, 2019, at 09:31, Julien Grall <julien.grall@arm.com> wrote:
>> On 19/07/2019 14:24, Julien Grall wrote:
>> Hi Tamas,
>>> On 19/07/2019 14:14, Tamas K Lengyel wrote:
>>>> On Fri, Jul 19, 2019 at 7:11 AM Julien Grall <julien.grall@arm.com> wrote:
>>>> 
>>>> Hi Tamas,
>>>> 
>>>>> On 19/07/2019 14:00, Tamas K Lengyel wrote:
>>>>>> On Fri, Jul 19, 2019 at 2:43 AM Julien Grall <julien.grall@arm.com> wrote:
>>>>>> 
>>>>>> Hi Tamas,
>>>>>> 
>>>>>> On 18/07/2019 18:48, Tamas K Lengyel wrote:
>>>>>>>>       - Line 1025: The tools needs to be able to deal for_each_vcpu(...) & co.
>>>>>>> 
>>>>>>> These can be made OK by adding braces. Other than that the only way I
>>>>>>> found to make it not change the indentation is to add the comment "/*
>>>>>>> *INDENT-OFF* */" before the block and "/* *INDENT-ON* */" afterwards.
>>>>>> 
>>>>>> None of them looks really appealing because it means astyle will not correctly
>>>>>> indent if the user does not add braces or comments.
>>>>>> 
>>>>>> Could astyle be easily modified to recognize foreach macros?
>>>>> 
>>>>> Not that I'm aware of. If you don't want to manually annotate files
>>>>> with unsupported macros then just exclude those files from astyle. I
>>>>> wouldn't recommend adding this to the CI for all files, only for those
>>>>> that their respective maintainers have confirmed to conform to the
>>>>> style and want to enforce it going forward.
>>>> 
>>>> So a couple use of an unsupported macros would make impossible to enforce the
>>>> coding style. This is not a very ideal position to be in.
>>>> 
>>>> _if_ we are going to adopt astyle then we need to be able to enforce it on every
>>>> Xen files long-term. If it is not possible to do it with astyle, then maybe this
>>>> is not the right tool to use.
>>>> 
>>>> For instance, I know that tools such as clang-format is able to deal with
>>>> foreach macros.
>>> 
>>> If there are better tools then sure, I don't really mind using
>>> something else. I just don't have time to do the manual style check
>>> back-and-forth anymore, so the sooner we have something in place the
>>> better.
>> I totally agree we need a tool so the reviewer can free-up some time to focus on more important things. However, I think we should be careful on what we adopt here.
>> Similar to Andrew, I am open with modifying the coding style to help the automatic style check. But I am not happy to disable automatic style on part (or entire) of files forever.
>> At the moment, clang-format feels more powerful and there are people working on it.
> 
> FYI, below a link to the clang-format changes:
> 
> https://github.com/xen-troops/Xen-Clang-format/blob/devel/clang-format.patch

Were these clang-format changes done for FuSa work?  Are they intended to be run within OSStest and/or Xen's Gitlab CI, which do not currently support OpenEmbedded/Yocto and xen-troops?

It would be helpful to have a xen-devel thread on the motivation for the clang-format work, the specific style being enforced (including the nuances discussed in this thread) and additional work needed before clang-format can perform automated style checking to address (a) existing Xen/Linux style requirements, (b) FuSa requirements.

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

Hi Rich,

On 19/07/2019 14:50, Rich Persaud wrote:
> On Jul 19, 2019, at 09:31, Julien Grall <julien.grall@arm.com> wrote:
>>> On 19/07/2019 14:24, Julien Grall wrote:
>>> Hi Tamas,
>>>> On 19/07/2019 14:14, Tamas K Lengyel wrote:
>>>>> On Fri, Jul 19, 2019 at 7:11 AM Julien Grall <julien.grall@arm.com> wrote:
>>>>>
>>>>> Hi Tamas,
>>>>>
>>>>>> On 19/07/2019 14:00, Tamas K Lengyel wrote:
>>>>>>> On Fri, Jul 19, 2019 at 2:43 AM Julien Grall <julien.grall@arm.com> wrote:
>>>>>>>
>>>>>>> Hi Tamas,
>>>>>>>
>>>>>>> On 18/07/2019 18:48, Tamas K Lengyel wrote:
>>>>>>>>>        - Line 1025: The tools needs to be able to deal for_each_vcpu(...) & co.
>>>>>>>>
>>>>>>>> These can be made OK by adding braces. Other than that the only way I
>>>>>>>> found to make it not change the indentation is to add the comment "/*
>>>>>>>> *INDENT-OFF* */" before the block and "/* *INDENT-ON* */" afterwards.
>>>>>>>
>>>>>>> None of them looks really appealing because it means astyle will not correctly
>>>>>>> indent if the user does not add braces or comments.
>>>>>>>
>>>>>>> Could astyle be easily modified to recognize foreach macros?
>>>>>>
>>>>>> Not that I'm aware of. If you don't want to manually annotate files
>>>>>> with unsupported macros then just exclude those files from astyle. I
>>>>>> wouldn't recommend adding this to the CI for all files, only for those
>>>>>> that their respective maintainers have confirmed to conform to the
>>>>>> style and want to enforce it going forward.
>>>>>
>>>>> So a couple use of an unsupported macros would make impossible to enforce the
>>>>> coding style. This is not a very ideal position to be in.
>>>>>
>>>>> _if_ we are going to adopt astyle then we need to be able to enforce it on every
>>>>> Xen files long-term. If it is not possible to do it with astyle, then maybe this
>>>>> is not the right tool to use.
>>>>>
>>>>> For instance, I know that tools such as clang-format is able to deal with
>>>>> foreach macros.
>>>>
>>>> If there are better tools then sure, I don't really mind using
>>>> something else. I just don't have time to do the manual style check
>>>> back-and-forth anymore, so the sooner we have something in place the
>>>> better.
>>> I totally agree we need a tool so the reviewer can free-up some time to focus on more important things. However, I think we should be careful on what we adopt here.
>>> Similar to Andrew, I am open with modifying the coding style to help the automatic style check. But I am not happy to disable automatic style on part (or entire) of files forever.
>>> At the moment, clang-format feels more powerful and there are people working on it.
>>
>> FYI, below a link to the clang-format changes:
>>
>> https://github.com/xen-troops/Xen-Clang-format/blob/devel/clang-format.patch
> 
> Were these clang-format changes done for FuSa work?  Are they intended to be run within OSStest and/or Xen's Gitlab CI, which do not currently support OpenEmbedded/Yocto and xen-troops?

It was originally started by EPAM to address review burden. IIRC, the goal is to 
have a robot checking coding style before any review is actually done. So 
probably part of Xen's Gitlab.

I am not sure why you mention OpenEmbedded/Yocto or even EPAM's Xen. The patch 
is against clang-format (from LLVM project) and does not have any dependencies 
on all the rest.

> 
> It would be helpful to have a xen-devel thread on the motivation for the clang-format work, the specific style being enforced (including the nuances discussed in this thread) and additional work needed before clang-format can perform automated style checking to address (a) existing Xen/Linux style requirements, (b) FuSa requirements.

I will leave that to Lars and Artem. They have been following the work more closely.

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Lars Kurth]

On 19/07/2019, 14:50, "Rich Persaud" <persaur@gmail.com> wrote:

    On Jul 19, 2019, at 09:31, Julien Grall <julien.grall@arm.com> wrote:
    >> On 19/07/2019 14:24, Julien Grall wrote:
    >> Hi Tamas,
    >>> On 19/07/2019 14:14, Tamas K Lengyel wrote:
    >>>> On Fri, Jul 19, 2019 at 7:11 AM Julien Grall <julien.grall@arm.com> wrote:
    >>>> 
    >>>> Hi Tamas,
    >>>> 
    >>>>> On 19/07/2019 14:00, Tamas K Lengyel wrote:
    >>>>>> On Fri, Jul 19, 2019 at 2:43 AM Julien Grall <julien.grall@arm.com> wrote:
    >>>>>> 
    >>>>>> Hi Tamas,
    >>>>>> 
    >>>>>> On 18/07/2019 18:48, Tamas K Lengyel wrote:
    >>>>>>>>       - Line 1025: The tools needs to be able to deal for_each_vcpu(...) & co.
    >>>>>>> 
    >>>>>>> These can be made OK by adding braces. Other than that the only way I
    >>>>>>> found to make it not change the indentation is to add the comment "/*
    >>>>>>> *INDENT-OFF* */" before the block and "/* *INDENT-ON* */" afterwards.
    >>>>>> 
    >>>>>> None of them looks really appealing because it means astyle will not correctly
    >>>>>> indent if the user does not add braces or comments.
    >>>>>> 
    >>>>>> Could astyle be easily modified to recognize foreach macros?
    >>>>> 
    >>>>> Not that I'm aware of. If you don't want to manually annotate files
    >>>>> with unsupported macros then just exclude those files from astyle. I
    >>>>> wouldn't recommend adding this to the CI for all files, only for those
    >>>>> that their respective maintainers have confirmed to conform to the
    >>>>> style and want to enforce it going forward.
    >>>> 
    >>>> So a couple use of an unsupported macros would make impossible to enforce the
    >>>> coding style. This is not a very ideal position to be in.
    >>>> 
    >>>> _if_ we are going to adopt astyle then we need to be able to enforce it on every
    >>>> Xen files long-term. If it is not possible to do it with astyle, then maybe this
    >>>> is not the right tool to use.
    >>>> 
    >>>> For instance, I know that tools such as clang-format is able to deal with
    >>>> foreach macros.
    >>> 
    >>> If there are better tools then sure, I don't really mind using
    >>> something else. I just don't have time to do the manual style check
    >>> back-and-forth anymore, so the sooner we have something in place the
    >>> better.
    >> I totally agree we need a tool so the reviewer can free-up some time to focus on more important things. However, I think we should be careful on what we adopt here.
    >> Similar to Andrew, I am open with modifying the coding style to help the automatic style check. But I am not happy to disable automatic style on part (or entire) of files forever.
    >> At the moment, clang-format feels more powerful and there are people working on it.
    > 
    > FYI, below a link to the clang-format changes:
    > 
    > https://github.com/xen-troops/Xen-Clang-format/blob/devel/clang-format.patch
    
    Were these clang-format changes done for FuSa work?  Are they intended to be run within OSStest and/or Xen's Gitlab CI, which do not currently support OpenEmbedded/Yocto and xen-troops?

No, they were done following a discussion at a previous summit and then nothing moved for two years
The idea was to avoid reviewers having to do mechanical tasks such as review styling
    
    It would be helpful to have a xen-devel thread on the motivation for the clang-format work, the specific style being enforced (including the nuances discussed in this thread) and additional work needed before clang-format can perform automated style checking to address (a) existing Xen/Linux style requirements, (b) FuSa requirements.

Ultimately, we want to integrate this into the CIv2 we discussed and agreed at the summit. Aka an automatic style check triggered by a mailing list post alongside build checks and basic smoke tests on top of QEMU. I still need to get on top of the notes

Lars
P.S.: Sorry, catching up on mailing list and stuff after a 2 week trip

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

On Fri, Jul 19, 2019 at 7:31 AM Julien Grall <julien.grall@arm.com> wrote:
>
>
>
> On 19/07/2019 14:24, Julien Grall wrote:
> > Hi Tamas,
> >
> > On 19/07/2019 14:14, Tamas K Lengyel wrote:
> >> On Fri, Jul 19, 2019 at 7:11 AM Julien Grall <julien.grall@arm.com> wrote:
> >>>
> >>> Hi Tamas,
> >>>
> >>> On 19/07/2019 14:00, Tamas K Lengyel wrote:
> >>>> On Fri, Jul 19, 2019 at 2:43 AM Julien Grall <julien.grall@arm.com> wrote:
> >>>>>
> >>>>> Hi Tamas,
> >>>>>
> >>>>> On 18/07/2019 18:48, Tamas K Lengyel wrote:
> >>>>>>>       - Line 1025: The tools needs to be able to deal for_each_vcpu(...)
> >>>>>>> & co.
> >>>>>>
> >>>>>> These can be made OK by adding braces. Other than that the only way I
> >>>>>> found to make it not change the indentation is to add the comment "/*
> >>>>>> *INDENT-OFF* */" before the block and "/* *INDENT-ON* */" afterwards.
> >>>>>
> >>>>> None of them looks really appealing because it means astyle will not correctly
> >>>>> indent if the user does not add braces or comments.
> >>>>>
> >>>>> Could astyle be easily modified to recognize foreach macros?
> >>>>
> >>>> Not that I'm aware of. If you don't want to manually annotate files
> >>>> with unsupported macros then just exclude those files from astyle. I
> >>>> wouldn't recommend adding this to the CI for all files, only for those
> >>>> that their respective maintainers have confirmed to conform to the
> >>>> style and want to enforce it going forward.
> >>>
> >>> So a couple use of an unsupported macros would make impossible to enforce the
> >>> coding style. This is not a very ideal position to be in.
> >>>
> >>> _if_ we are going to adopt astyle then we need to be able to enforce it on every
> >>> Xen files long-term. If it is not possible to do it with astyle, then maybe this
> >>> is not the right tool to use.
> >>>
> >>> For instance, I know that tools such as clang-format is able to deal with
> >>> foreach macros.
> >>
> >> If there are better tools then sure, I don't really mind using
> >> something else. I just don't have time to do the manual style check
> >> back-and-forth anymore, so the sooner we have something in place the
> >> better.
> >
> > I totally agree we need a tool so the reviewer can free-up some time to focus on
> > more important things. However, I think we should be careful on what we adopt here.
> >
> > Similar to Andrew, I am open with modifying the coding style to help the
> > automatic style check. But I am not happy to disable automatic style on part (or
> > entire) of files forever.
> >
> > At the moment, clang-format feels more powerful and there are people working on it.
>
> FYI, below a link to the clang-format changes:
>
> https://github.com/xen-troops/Xen-Clang-format/blob/devel/clang-format.patch
>

Thanks, I'll give this a shot. Since this requires patching clang it
looks to me like it may be a while before it's generally available
downstream. Perhaps it's still worth exploring adding .astylerc and
have at least partial coverage for the automated style checks for the
interim.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Viktor Mitin]

Please be aware that such for_each_* cases are covered by clang-format.

Thanks

On Thu, Jul 18, 2019 at 8:50 PM Tamas K Lengyel <tamas@tklengyel.com> wrote:
>
> >    - Line 1025: The tools needs to be able to deal for_each_vcpu(...) & co.
>
> These can be made OK by adding braces. Other than that the only way I
> found to make it not change the indentation is to add the comment "/*
> *INDENT-OFF* */" before the block and "/* *INDENT-ON* */" afterwards.
>
> Tamas
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Viktor Mitin]

Hi Julien, All,

On Thu, Jul 18, 2019 at 6:44 PM Julien Grall <julien.grall@arm.com> wrote:
>
> Hi Tamas,
>
> On 7/18/19 4:14 PM, Tamas K Lengyel wrote:
> > On Thu, Jul 18, 2019 at 9:02 AM Julien Grall <julien.grall@arm.com> wrote:
> >>
> >> Hi Tamas,
> >>
> >> Adding Lars, Artem and Iurii. Iurii has been working on a version for
> >> clang-format recently.
> >>
> >> On 7/18/19 3:43 PM, Tamas K Lengyel wrote:
> >>> Using astyle (http://astyle.sourceforge.net) can greatly reduce the overhead of
> >>> manually checking and applying style-fixes to source-code. The included
> >>> .astylerc is the closest approximation of the established Xen style (including
> >>> styles not formally spelled out by CODING_STYLE but commonly requested).
> >>>
> >>> Checking the comment styles are not included in the automation.
> >>>
> >>> Incorporating Xen's exception to the do-while style is only partially possible,
> >>> thus a change is proposed to the CODING_STYLE of moving the brace from "do {"
> >>> to the next line.
> >>>
> >>> Most of Xen's code-base is non-conforming at the moment: 289 files pass
> >>> unchanged, 876 have some style issue
> >>>
> >>> Ideally we can slowly migrate the entire code-base to be conforming, thus
> >>> eliminating the need of discussing and enforcing style issues manually on the
> >>> mailinglist.
> >>
> >> I quite like the idea of an automatic coding style checker. However, it
> >> is a bit concerning that not even a 1/3 of the files are able to pass
> >> the coding style you suggest. Could you explain whether this is because
> >> the files does not already follow Xen coding style or is it just the
> >> difference with astyle?
> >>
> >> What are the main style issues?
> >
> > Looks like a lot of files that don't follow the Xen coding style
> > as-is. Alignment issues seem to me to be the most common errors. See
> > the full diff here:
> >
> > https://gist.github.com/tklengyel/c5cac14a0d57f119dd7747a1be6fb260
> >
> > We can perhaps tune some aspects of it we disagree with the astyle
> > generated style and try to override it. I did my best to make it
> > conform to the existing Xen style but certainly there could be other
> > tweaks made to reduce the churn.
>
> I think we definitely want to avoid churn as this is going to take a lot
> of time to fix all the places to the new indentation.
>
> Going through the diff I can see major differences with the Xen Coding
> style and also what looks like inconsistencies from the tools itself:
>    - Line 58: This is fairly common to indent the parameters as it is
> today. But then on line 158/272 it indents as we do today. So I am not
> sure what the expected coding style from the tools.
>    - Line 67: I believe Jan request the space before label
Seems agreed not to add the spaces before label. Right?

>    - Line 139: The { are commonly on the same line as struct or definition.
This should be stated in the Coding style explicitly.

>    - Line 276: The switch case indentation was correct from Xen PoV before
>    - Line 289: Files imported from Linux should not be touch here.

The code files to style mapping to be automated outside of clang-format tool.
Clang-format tool supports  3 new formatting styles to cover all the
cases mentioned in Xen coding style document: Xen, Libxl,Linux

>    - Line 1025: The tools needs to be able to deal for_each_vcpu(...) & co.
Clang-format tool supports such cases.

>    - Line 8735: It looks like to me the tools policy is quite
> inconsistent. In previous place it keeps it properly aligned see line 5777.
>
> I have only looked quickly through the diff, but I think they are the
> main one that should probably be resolved.
>

Please be aware that it is important to add all the cases mentioned
above (and all the other) to the Xen Coding style document explicitly.
This seems the biggest non-technical issue to overcome...

> Cheers,
>
> --
> Julien Grall
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

lOn 26/07/2019 15:49, Viktor Mitin wrote:
> Hi Julien, All,

Hi,

> On Thu, Jul 18, 2019 at 6:44 PM Julien Grall <julien.grall@arm.com> wrote:
>>
>> Hi Tamas,
>>
>> On 7/18/19 4:14 PM, Tamas K Lengyel wrote:
>>> On Thu, Jul 18, 2019 at 9:02 AM Julien Grall <julien.grall@arm.com> wrote:
>>>>
>>>> Hi Tamas,
>>>>
>>>> Adding Lars, Artem and Iurii. Iurii has been working on a version for
>>>> clang-format recently.
>>>>
>>>> On 7/18/19 3:43 PM, Tamas K Lengyel wrote:
>>>>> Using astyle (http://astyle.sourceforge.net) can greatly reduce the overhead of
>>>>> manually checking and applying style-fixes to source-code. The included
>>>>> .astylerc is the closest approximation of the established Xen style (including
>>>>> styles not formally spelled out by CODING_STYLE but commonly requested).
>>>>>
>>>>> Checking the comment styles are not included in the automation.
>>>>>
>>>>> Incorporating Xen's exception to the do-while style is only partially possible,
>>>>> thus a change is proposed to the CODING_STYLE of moving the brace from "do {"
>>>>> to the next line.
>>>>>
>>>>> Most of Xen's code-base is non-conforming at the moment: 289 files pass
>>>>> unchanged, 876 have some style issue
>>>>>
>>>>> Ideally we can slowly migrate the entire code-base to be conforming, thus
>>>>> eliminating the need of discussing and enforcing style issues manually on the
>>>>> mailinglist.
>>>>
>>>> I quite like the idea of an automatic coding style checker. However, it
>>>> is a bit concerning that not even a 1/3 of the files are able to pass
>>>> the coding style you suggest. Could you explain whether this is because
>>>> the files does not already follow Xen coding style or is it just the
>>>> difference with astyle?
>>>>
>>>> What are the main style issues?
>>>
>>> Looks like a lot of files that don't follow the Xen coding style
>>> as-is. Alignment issues seem to me to be the most common errors. See
>>> the full diff here:
>>>
>>> https://gist.github.com/tklengyel/c5cac14a0d57f119dd7747a1be6fb260
>>>
>>> We can perhaps tune some aspects of it we disagree with the astyle
>>> generated style and try to override it. I did my best to make it
>>> conform to the existing Xen style but certainly there could be other
>>> tweaks made to reduce the churn.
>>
>> I think we definitely want to avoid churn as this is going to take a lot
>> of time to fix all the places to the new indentation.
>>
>> Going through the diff I can see major differences with the Xen Coding
>> style and also what looks like inconsistencies from the tools itself:
>>     - Line 58: This is fairly common to indent the parameters as it is
>> today. But then on line 158/272 it indents as we do today. So I am not
>> sure what the expected coding style from the tools.
>>     - Line 67: I believe Jan request the space before label
> Seems agreed not to add the spaces before label. Right?

I don't remember any agreement on this. Please point to the thread.

> 
>>     - Line 139: The { are commonly on the same line as struct or definition.
> This should be stated in the Coding style explicitly.

[...]

> Please be aware that it is important to add all the cases mentioned
> above (and all the other) to the Xen Coding style document explicitly.
> This seems the biggest non-technical issue to overcome...

Thank you for the reminder. However, someone needs to drive it... Are you going 
to do that?

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Jan Beulich]

On 26.07.2019 16:49, Viktor Mitin wrote:
> Hi Julien, All,
> 
> On Thu, Jul 18, 2019 at 6:44 PM Julien Grall <julien.grall@arm.com> wrote:
>>
>> Hi Tamas,
>>
>> On 7/18/19 4:14 PM, Tamas K Lengyel wrote:
>>> On Thu, Jul 18, 2019 at 9:02 AM Julien Grall <julien.grall@arm.com> wrote:
>>>>
>>>> Hi Tamas,
>>>>
>>>> Adding Lars, Artem and Iurii. Iurii has been working on a version for
>>>> clang-format recently.
>>>>
>>>> On 7/18/19 3:43 PM, Tamas K Lengyel wrote:
>>>>> Using astyle (http://astyle.sourceforge.net) can greatly reduce the overhead of
>>>>> manually checking and applying style-fixes to source-code. The included
>>>>> .astylerc is the closest approximation of the established Xen style (including
>>>>> styles not formally spelled out by CODING_STYLE but commonly requested).
>>>>>
>>>>> Checking the comment styles are not included in the automation.
>>>>>
>>>>> Incorporating Xen's exception to the do-while style is only partially possible,
>>>>> thus a change is proposed to the CODING_STYLE of moving the brace from "do {"
>>>>> to the next line.
>>>>>
>>>>> Most of Xen's code-base is non-conforming at the moment: 289 files pass
>>>>> unchanged, 876 have some style issue
>>>>>
>>>>> Ideally we can slowly migrate the entire code-base to be conforming, thus
>>>>> eliminating the need of discussing and enforcing style issues manually on the
>>>>> mailinglist.
>>>>
>>>> I quite like the idea of an automatic coding style checker. However, it
>>>> is a bit concerning that not even a 1/3 of the files are able to pass
>>>> the coding style you suggest. Could you explain whether this is because
>>>> the files does not already follow Xen coding style or is it just the
>>>> difference with astyle?
>>>>
>>>> What are the main style issues?
>>>
>>> Looks like a lot of files that don't follow the Xen coding style
>>> as-is. Alignment issues seem to me to be the most common errors. See
>>> the full diff here:
>>>
>>> https://gist.github.com/tklengyel/c5cac14a0d57f119dd7747a1be6fb260
>>>
>>> We can perhaps tune some aspects of it we disagree with the astyle
>>> generated style and try to override it. I did my best to make it
>>> conform to the existing Xen style but certainly there could be other
>>> tweaks made to reduce the churn.
>>
>> I think we definitely want to avoid churn as this is going to take a lot
>> of time to fix all the places to the new indentation.
>>
>> Going through the diff I can see major differences with the Xen Coding
>> style and also what looks like inconsistencies from the tools itself:
>>     - Line 58: This is fairly common to indent the parameters as it is
>> today. But then on line 158/272 it indents as we do today. So I am not
>> sure what the expected coding style from the tools.
>>     - Line 67: I believe Jan request the space before label
> Seems agreed not to add the spaces before label. Right?

Certainly not, afaia. I will object to any written down rule disallowing
leading blank(s) altogether. I will argue for making mandatory at least
one blank of indentation.

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

Hi Jan,

On 7/29/19 1:19 PM, Jan Beulich wrote:
> On 26.07.2019 16:49, Viktor Mitin wrote:
>> Hi Julien, All,
>>
>> On Thu, Jul 18, 2019 at 6:44 PM Julien Grall <julien.grall@arm.com> wrote:
>>>
>>> Hi Tamas,
>>>
>>> On 7/18/19 4:14 PM, Tamas K Lengyel wrote:
>>>> On Thu, Jul 18, 2019 at 9:02 AM Julien Grall <julien.grall@arm.com> wrote:
>>>>>
>>>>> Hi Tamas,
>>>>>
>>>>> Adding Lars, Artem and Iurii. Iurii has been working on a version for
>>>>> clang-format recently.
>>>>>
>>>>> On 7/18/19 3:43 PM, Tamas K Lengyel wrote:
>>>>>> Using astyle (http://astyle.sourceforge.net) can greatly reduce the overhead of
>>>>>> manually checking and applying style-fixes to source-code. The included
>>>>>> .astylerc is the closest approximation of the established Xen style (including
>>>>>> styles not formally spelled out by CODING_STYLE but commonly requested).
>>>>>>
>>>>>> Checking the comment styles are not included in the automation.
>>>>>>
>>>>>> Incorporating Xen's exception to the do-while style is only partially possible,
>>>>>> thus a change is proposed to the CODING_STYLE of moving the brace from "do {"
>>>>>> to the next line.
>>>>>>
>>>>>> Most of Xen's code-base is non-conforming at the moment: 289 files pass
>>>>>> unchanged, 876 have some style issue
>>>>>>
>>>>>> Ideally we can slowly migrate the entire code-base to be conforming, thus
>>>>>> eliminating the need of discussing and enforcing style issues manually on the
>>>>>> mailinglist.
>>>>>
>>>>> I quite like the idea of an automatic coding style checker. However, it
>>>>> is a bit concerning that not even a 1/3 of the files are able to pass
>>>>> the coding style you suggest. Could you explain whether this is because
>>>>> the files does not already follow Xen coding style or is it just the
>>>>> difference with astyle?
>>>>>
>>>>> What are the main style issues?
>>>>
>>>> Looks like a lot of files that don't follow the Xen coding style
>>>> as-is. Alignment issues seem to me to be the most common errors. See
>>>> the full diff here:
>>>>
>>>> https://gist.github.com/tklengyel/c5cac14a0d57f119dd7747a1be6fb260
>>>>
>>>> We can perhaps tune some aspects of it we disagree with the astyle
>>>> generated style and try to override it. I did my best to make it
>>>> conform to the existing Xen style but certainly there could be other
>>>> tweaks made to reduce the churn.
>>>
>>> I think we definitely want to avoid churn as this is going to take a lot
>>> of time to fix all the places to the new indentation.
>>>
>>> Going through the diff I can see major differences with the Xen Coding
>>> style and also what looks like inconsistencies from the tools itself:
>>>      - Line 58: This is fairly common to indent the parameters as it is
>>> today. But then on line 158/272 it indents as we do today. So I am not
>>> sure what the expected coding style from the tools.
>>>      - Line 67: I believe Jan request the space before label
>> Seems agreed not to add the spaces before label. Right?
> 
> Certainly not, afaia. I will object to any written down rule disallowing
> leading blank(s) altogether. I will argue for making mandatory at least
> one blank of indentation.

Coding style are a matter of taste. If everyone is going to say "I want 
this in the coding style", then we are going to spend countless of hours 
bike-shedding. This is not how we should use our already limited time.

If we want to re-use existing checker, then we will most likely have to 
make some compromise. I am not suggesting this should be (or not be) 
part of the compromises.

Cheers,


-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Jan Beulich]

On 29.07.2019 14:49, Julien Grall wrote:
> Hi Jan,
> 
> On 7/29/19 1:19 PM, Jan Beulich wrote:
>> On 26.07.2019 16:49, Viktor Mitin wrote:
>>> Hi Julien, All,
>>>
>>> On Thu, Jul 18, 2019 at 6:44 PM Julien Grall <julien.grall@arm.com> wrote:
>>>>
>>>> Hi Tamas,
>>>>
>>>> On 7/18/19 4:14 PM, Tamas K Lengyel wrote:
>>>>> On Thu, Jul 18, 2019 at 9:02 AM Julien Grall <julien.grall@arm.com> wrote:
>>>>>>
>>>>>> Hi Tamas,
>>>>>>
>>>>>> Adding Lars, Artem and Iurii. Iurii has been working on a version for
>>>>>> clang-format recently.
>>>>>>
>>>>>> On 7/18/19 3:43 PM, Tamas K Lengyel wrote:
>>>>>>> Using astyle (http://astyle.sourceforge.net) can greatly reduce the overhead of
>>>>>>> manually checking and applying style-fixes to source-code. The included
>>>>>>> .astylerc is the closest approximation of the established Xen style (including
>>>>>>> styles not formally spelled out by CODING_STYLE but commonly requested).
>>>>>>>
>>>>>>> Checking the comment styles are not included in the automation.
>>>>>>>
>>>>>>> Incorporating Xen's exception to the do-while style is only partially possible,
>>>>>>> thus a change is proposed to the CODING_STYLE of moving the brace from "do {"
>>>>>>> to the next line.
>>>>>>>
>>>>>>> Most of Xen's code-base is non-conforming at the moment: 289 files pass
>>>>>>> unchanged, 876 have some style issue
>>>>>>>
>>>>>>> Ideally we can slowly migrate the entire code-base to be conforming, thus
>>>>>>> eliminating the need of discussing and enforcing style issues manually on the
>>>>>>> mailinglist.
>>>>>>
>>>>>> I quite like the idea of an automatic coding style checker. However, it
>>>>>> is a bit concerning that not even a 1/3 of the files are able to pass
>>>>>> the coding style you suggest. Could you explain whether this is because
>>>>>> the files does not already follow Xen coding style or is it just the
>>>>>> difference with astyle?
>>>>>>
>>>>>> What are the main style issues?
>>>>>
>>>>> Looks like a lot of files that don't follow the Xen coding style
>>>>> as-is. Alignment issues seem to me to be the most common errors. See
>>>>> the full diff here:
>>>>>
>>>>> https://gist.github.com/tklengyel/c5cac14a0d57f119dd7747a1be6fb260
>>>>>
>>>>> We can perhaps tune some aspects of it we disagree with the astyle
>>>>> generated style and try to override it. I did my best to make it
>>>>> conform to the existing Xen style but certainly there could be other
>>>>> tweaks made to reduce the churn.
>>>>
>>>> I think we definitely want to avoid churn as this is going to take a lot
>>>> of time to fix all the places to the new indentation.
>>>>
>>>> Going through the diff I can see major differences with the Xen Coding
>>>> style and also what looks like inconsistencies from the tools itself:
>>>>      - Line 58: This is fairly common to indent the parameters as it is
>>>> today. But then on line 158/272 it indents as we do today. So I am not
>>>> sure what the expected coding style from the tools.
>>>>      - Line 67: I believe Jan request the space before label
>>> Seems agreed not to add the spaces before label. Right?
>>
>> Certainly not, afaia. I will object to any written down rule disallowing
>> leading blank(s) altogether. I will argue for making mandatory at least
>> one blank of indentation.
> 
> Coding style are a matter of taste. If everyone is going to say "I want
> this in the coding style", then we are going to spend countless of hours
> bike-shedding. This is not how we should use our already limited time.

I agree with what you say in general, but not in this specific case: I've
explained why the leading blank(s) are wanted here. This is not because of
my taste, but because of helping with patch review.

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Viktor Mitin]

Hi Jan,

On Mon, Jul 29, 2019 at 4:21 PM Jan Beulich <JBeulich@suse.com> wrote:

> >>>>      - Line 67: I believe Jan request the space before label
> >>> Seems agreed not to add the spaces before label. Right?
> >>
> >> Certainly not, afaia. I will object to any written down rule disallowing
> >> leading blank(s) altogether. I will argue for making mandatory at least
> >> one blank of indentation.
> >
> > Coding style are a matter of taste. If everyone is going to say "I want
> > this in the coding style", then we are going to spend countless of hours
> > bike-shedding. This is not how we should use our already limited time.
>
> I agree with what you say in general, but not in this specific case: I've
> explained why the leading blank(s) are wanted here. This is not because of
> my taste, but because of helping with patch review.


I've checked all the styles supported by clang-format at the moment:
LLVM, Google, Chromium, Mozilla, WebKit, Microsoft. None of them uses
spaces before labels. It seems Linux coding style does not use it as
well. Please see the questions below:

How all those projects live without this issue?
What is the reason not to fix 'diff -p'? Is it not possible at all?
Could you please share more details about the background of the issue
and examples?

Thanks

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Jan Beulich]

On 31.07.2019 18:20, Viktor Mitin wrote:
> How all those projects live without this issue?

Perhaps they don't care? I do.

> What is the reason not to fix 'diff -p'? Is it not possible at all?

I have no idea, but I guess there's a reason for its current
behavior.

> Could you please share more details about the background of the issue
> and examples?

What (further) background and what kind of examples are you after?

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Viktor Mitin]

On Thu, Aug 1, 2019 at 10:40 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> On 31.07.2019 18:20, Viktor Mitin wrote:
> > How all those projects live without this issue?
>
> Perhaps they don't care? I do.
>
> > What is the reason not to fix 'diff -p'? Is it not possible at all?
>
> I have no idea, but I guess there's a reason for its current
> behavior.

Jan, it seems it is not a good idea to modify the Xen coding style
without investigation of the issue first. It takes some effort to
automate every such (or similar) exceptional code formatting case. So
it looks we could find a better option, than just adding one more
exception to Xen coding style, at least worth to investigate it first.

> > Could you please share more details about the background of the issue
> > and examples?
>
> What (further) background and what kind of examples are you after?

I expected you to have some arguments why it cannot be fixed in the
diff or other tools.
The examples I meant are any real patches code examples to be used for
this investigation.
Hopefully, you care about it, as you mentioned previously.

Thanks

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Jan Beulich]

On 01.08.2019 14:16, Viktor Mitin wrote:
> I expected you to have some arguments why it cannot be fixed in the
> diff or other tools.

I don't follow: How does it matter here whether I have arguments towards
(not) fixing diff etc? That's their maintainers' arguments, if any, not
mine. All I know (from the last time we've been discussing this) is that
even up-to-date diff (at that time at least, i.e. a few months back)
still behaved that way. Which means if it was fixed today, it would
still be a few years out at least until we could assume old diff isn't
in use anymore.

> The examples I meant are any real patches code examples to be used for
> this investigation.

You can easily observe the anomaly yourself. You could also go hunt for
the previous discussion. To me, this discussion as a whole has already
taken far more time than it would take to "manually" comment on _many_
patches violating style. I'm fine to provide input here, but the
overhead time- wise needs to remain reasonable. And as much as I
appreciate you to have taken on this task, I think part of the decision
to do so should have been to familiarize yourself with prior discussions
on the subject.

Anyway - here is your example:

--- unstable.orig/xen/common/grant_table.c
+++ unstable/xen/common/grant_table.c
@@ -1573,6 +1573,7 @@ fault:
  
      for ( i = 0; i < partial_done; i++ )
          unmap_common_complete(&common[i]);
+    (void)0;
      return -EFAULT;
  }
  
Without the function name the code addition is ambiguous; line numbers
can't always be relied upon, and code review often can be done without
actually opening the file in question and looking at the patched code
location.

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Juergen Gross]

On 01.08.19 14:16, Viktor Mitin wrote:
> On Thu, Aug 1, 2019 at 10:40 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> On 31.07.2019 18:20, Viktor Mitin wrote:
>>> How all those projects live without this issue?
>>
>> Perhaps they don't care? I do.
>>
>>> What is the reason not to fix 'diff -p'? Is it not possible at all?
>>
>> I have no idea, but I guess there's a reason for its current
>> behavior.
> 
> Jan, it seems it is not a good idea to modify the Xen coding style
> without investigation of the issue first. It takes some effort to
> automate every such (or similar) exceptional code formatting case. So
> it looks we could find a better option, than just adding one more
> exception to Xen coding style, at least worth to investigate it first.
> 
>>> Could you please share more details about the background of the issue
>>> and examples?
>>
>> What (further) background and what kind of examples are you after?
> 
> I expected you to have some arguments why it cannot be fixed in the
> diff or other tools.

Changing diff upstream might be possible. Changing today's incarnations
in all distros out in the wild is barely doable.

Another question is whether we really want diff to be changed. The
current behavior is fine for assembly sources for example. A changed
diff would no longer present the last label for context.

> The examples I meant are any real patches code examples to be used for
> this investigation.

A snipplet from commit 57f8b13c724023c78fa15a80452d1de3e51a1418:

@@ -4096,14 +4097,12 @@ retry_transaction:
          goto out;

      if (target == NULL) {
-        libxl__xs_printf(gc, t, target_path, "%"PRIu32,
-                         (uint32_t) info.current_memkb);
-        *target_memkb = (uint32_t) info.current_memkb;
+        libxl__xs_printf(gc, t, target_path, "%"PRIu64, 
info.current_memkb);
+        *target_memkb = info.current_memkb;
      }
      if (staticmax == NULL) {
-        libxl__xs_printf(gc, t, max_path, "%"PRIu32,
-                         (uint32_t) info.max_memkb);
-        *max_memkb = (uint32_t) info.max_memkb;
+        libxl__xs_printf(gc, t, max_path, "%"PRIu64, info.max_memkb);
+        *max_memkb = info.max_memkb;
      }

      rc = 0;


Juergen


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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Viktor Mitin]

On Thu, Aug 1, 2019 at 3:50 PM Juergen Gross <jgross@suse.com> wrote:
>
> On 01.08.19 14:16, Viktor Mitin wrote:
> > On Thu, Aug 1, 2019 at 10:40 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>
> >> On 31.07.2019 18:20, Viktor Mitin wrote:
> >>> How all those projects live without this issue?
> >>
> >> Perhaps they don't care? I do.
> >>
> >>> What is the reason not to fix 'diff -p'? Is it not possible at all?
> >>
> >> I have no idea, but I guess there's a reason for its current
> >> behavior.
> >
> > Jan, it seems it is not a good idea to modify the Xen coding style
> > without investigation of the issue first. It takes some effort to
> > automate every such (or similar) exceptional code formatting case. So
> > it looks we could find a better option, than just adding one more
> > exception to Xen coding style, at least worth to investigate it first.
> >
> >>> Could you please share more details about the background of the issue
> >>> and examples?
> >>
> >> What (further) background and what kind of examples are you after?
> >
> > I expected you to have some arguments why it cannot be fixed in the
> > diff or other tools.
>
> Changing diff upstream might be possible. Changing today's incarnations
> in all distros out in the wild is barely doable.
>
> Another question is whether we really want diff to be changed. The
> current behavior is fine for assembly sources for example. A changed
> diff would no longer present the last label for context.
>
> > The examples I meant are any real patches code examples to be used for
> > this investigation.
>
> A snipplet from commit 57f8b13c724023c78fa15a80452d1de3e51a1418:
>
> @@ -4096,14 +4097,12 @@ retry_transaction:
>           goto out;
>
>       if (target == NULL) {
> -        libxl__xs_printf(gc, t, target_path, "%"PRIu32,
> -                         (uint32_t) info.current_memkb);
> -        *target_memkb = (uint32_t) info.current_memkb;
> +        libxl__xs_printf(gc, t, target_path, "%"PRIu64,
> info.current_memkb);
> +        *target_memkb = info.current_memkb;
>       }
>       if (staticmax == NULL) {
> -        libxl__xs_printf(gc, t, max_path, "%"PRIu32,
> -                         (uint32_t) info.max_memkb);
> -        *max_memkb = (uint32_t) info.max_memkb;
> +        libxl__xs_printf(gc, t, max_path, "%"PRIu64, info.max_memkb);
> +        *max_memkb = info.max_memkb;
>       }
>
>       rc = 0;
>

I've build gnu diffutils latest release 3.7 and checked the code and
the issue. It seems this feature (-p) doesn't work properly and has
many more bugs than just the issue with labels. See the example below,
the change has been made in the function a1(), however, in the diff
another function is shown a(). There are more examples when it doesn't
work properly... I would not recommend relying on this feature since
it seems it is not well tested:

 1 c@cn ~/Downloads/diffutils-3.7$ ./src/diff --version
diff (GNU diffutils) 3.7
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <https://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by Paul Eggert, Mike Haertel, David Hayes,
Richard Stallman, and Len Tower.

0 c@cn ~/Downloads/diffutils-3.7$ cat ~/w/t.c
int func a()
{
    int a;
    int b;
}

int func a1(ttt, tttt, tttt)
{
    int a1;
}

0 c@cn ~/Downloads/diffutils-3.7$ cat ~/w/t1.c
afdasdfasdf

int func a()
{
    int a;
    int b;
}

int func a1(ttt, tttt, tttt)
{
    int a0;
}

0 c@cn ~/Downloads/diffutils-3.7$ ./src/diff -p ~/w/t.c ~/w/t1.c
*** /home/c/w/t.c       2019-08-02 11:24:42.044376084 +0300
--- /home/c/w/t1.c      2019-08-02 11:22:39.947610893 +0300
***************
*** 1,3 ****
--- 1,5 ----
+ afdasdfasdf
+
  int func a()
  {
      int a;
*************** int func a()
*** 6,11 ****

  int func a1(ttt, tttt, tttt)
  {
!     int a1;
  }

--- 8,13 ----

  int func a1(ttt, tttt, tttt)
  {
!     int a0;
  }



Thanks

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Juergen Gross]

On 02.08.19 10:38, Viktor Mitin wrote:
> On Thu, Aug 1, 2019 at 3:50 PM Juergen Gross <jgross@suse.com> wrote:
>>
>> On 01.08.19 14:16, Viktor Mitin wrote:
>>> On Thu, Aug 1, 2019 at 10:40 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>
>>>> On 31.07.2019 18:20, Viktor Mitin wrote:
>>>>> How all those projects live without this issue?
>>>>
>>>> Perhaps they don't care? I do.
>>>>
>>>>> What is the reason not to fix 'diff -p'? Is it not possible at all?
>>>>
>>>> I have no idea, but I guess there's a reason for its current
>>>> behavior.
>>>
>>> Jan, it seems it is not a good idea to modify the Xen coding style
>>> without investigation of the issue first. It takes some effort to
>>> automate every such (or similar) exceptional code formatting case. So
>>> it looks we could find a better option, than just adding one more
>>> exception to Xen coding style, at least worth to investigate it first.
>>>
>>>>> Could you please share more details about the background of the issue
>>>>> and examples?
>>>>
>>>> What (further) background and what kind of examples are you after?
>>>
>>> I expected you to have some arguments why it cannot be fixed in the
>>> diff or other tools.
>>
>> Changing diff upstream might be possible. Changing today's incarnations
>> in all distros out in the wild is barely doable.
>>
>> Another question is whether we really want diff to be changed. The
>> current behavior is fine for assembly sources for example. A changed
>> diff would no longer present the last label for context.
>>
>>> The examples I meant are any real patches code examples to be used for
>>> this investigation.
>>
>> A snipplet from commit 57f8b13c724023c78fa15a80452d1de3e51a1418:
>>
>> @@ -4096,14 +4097,12 @@ retry_transaction:
>>            goto out;
>>
>>        if (target == NULL) {
>> -        libxl__xs_printf(gc, t, target_path, "%"PRIu32,
>> -                         (uint32_t) info.current_memkb);
>> -        *target_memkb = (uint32_t) info.current_memkb;
>> +        libxl__xs_printf(gc, t, target_path, "%"PRIu64,
>> info.current_memkb);
>> +        *target_memkb = info.current_memkb;
>>        }
>>        if (staticmax == NULL) {
>> -        libxl__xs_printf(gc, t, max_path, "%"PRIu32,
>> -                         (uint32_t) info.max_memkb);
>> -        *max_memkb = (uint32_t) info.max_memkb;
>> +        libxl__xs_printf(gc, t, max_path, "%"PRIu64, info.max_memkb);
>> +        *max_memkb = info.max_memkb;
>>        }
>>
>>        rc = 0;
>>
> 
> I've build gnu diffutils latest release 3.7 and checked the code and
> the issue. It seems this feature (-p) doesn't work properly and has
> many more bugs than just the issue with labels. See the example below,
> the change has been made in the function a1(), however, in the diff
> another function is shown a().

This case is completely fine, as the context of the diff is starting
before the definition of a() (and just after a1()). This is important in
case you only add a new function for example.


Juergen

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Viktor Mitin]

On Fri, Aug 2, 2019 at 12:23 PM Juergen Gross <jgross@suse.com> wrote:

> >> A snipplet from commit 57f8b13c724023c78fa15a80452d1de3e51a1418:
> >>
> >> @@ -4096,14 +4097,12 @@ retry_transaction:
> >>            goto out;
> >>
> >>        if (target == NULL) {
> >> -        libxl__xs_printf(gc, t, target_path, "%"PRIu32,
> >> -                         (uint32_t) info.current_memkb);
> >> -        *target_memkb = (uint32_t) info.current_memkb;
> >> +        libxl__xs_printf(gc, t, target_path, "%"PRIu64,
> >> info.current_memkb);
> >> +        *target_memkb = info.current_memkb;
> >>        }
> >>        if (staticmax == NULL) {
> >> -        libxl__xs_printf(gc, t, max_path, "%"PRIu32,
> >> -                         (uint32_t) info.max_memkb);
> >> -        *max_memkb = (uint32_t) info.max_memkb;
> >> +        libxl__xs_printf(gc, t, max_path, "%"PRIu64, info.max_memkb);
> >> +        *max_memkb = info.max_memkb;
> >>        }
> >>
> >>        rc = 0;
> >>
> >
> > I've build gnu diffutils latest release 3.7 and checked the code and
> > the issue. It seems this feature (-p) doesn't work properly and has
> > many more bugs than just the issue with labels. See the example below,
> > the change has been made in the function a1(), however, in the diff
> > another function is shown a().
>
> This case is completely fine, as the context of the diff is starting
> before the definition of a() (and just after a1()). This is important in
> case you only add a new function for example.
>

Juergen, the diff returns wrong name of the function silently. I don't
think it is 'completely fine', I would rather call it a bug, or
non-documented limitation, but definitely not a feature. As I wrote
previously, I played with -p a little and observed more similar issues
with it. The reason is that the next diff code (see below) tries to
match function names using simple regexp for a line, assuming that
line with function name cannot start with a 'non-alpha' characters. So
even adding one more space before a line with function name breaks it.

diffutils-3.7/src/diff.c:462
    case 'p':
      show_c_function = true;
      add_regexp (&function_regexp_list, "^[[:alpha:]$_]");

It is probably could be improved by adding 'no : symbol at the end of
the line' logic to this regexp. It will resolve the issue with labels,
but will add one more bug (or limitation) about using ":" in such
lines.

The issue is more general here. The diff tool should not parse any
programming languages, it should just compare the strings.

Thanks

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Jan Beulich]

On 02.08.2019 13:44, Viktor Mitin wrote:
> On Fri, Aug 2, 2019 at 12:23 PM Juergen Gross <jgross@suse.com> wrote:
> 
>>>> A snipplet from commit 57f8b13c724023c78fa15a80452d1de3e51a1418:
>>>>
>>>> @@ -4096,14 +4097,12 @@ retry_transaction:
>>>>             goto out;
>>>>
>>>>         if (target == NULL) {
>>>> -        libxl__xs_printf(gc, t, target_path, "%"PRIu32,
>>>> -                         (uint32_t) info.current_memkb);
>>>> -        *target_memkb = (uint32_t) info.current_memkb;
>>>> +        libxl__xs_printf(gc, t, target_path, "%"PRIu64,
>>>> info.current_memkb);
>>>> +        *target_memkb = info.current_memkb;
>>>>         }
>>>>         if (staticmax == NULL) {
>>>> -        libxl__xs_printf(gc, t, max_path, "%"PRIu32,
>>>> -                         (uint32_t) info.max_memkb);
>>>> -        *max_memkb = (uint32_t) info.max_memkb;
>>>> +        libxl__xs_printf(gc, t, max_path, "%"PRIu64, info.max_memkb);
>>>> +        *max_memkb = info.max_memkb;
>>>>         }
>>>>
>>>>         rc = 0;
>>>>
>>>
>>> I've build gnu diffutils latest release 3.7 and checked the code and
>>> the issue. It seems this feature (-p) doesn't work properly and has
>>> many more bugs than just the issue with labels. See the example below,
>>> the change has been made in the function a1(), however, in the diff
>>> another function is shown a().
>>
>> This case is completely fine, as the context of the diff is starting
>> before the definition of a() (and just after a1()). This is important in
>> case you only add a new function for example.
>>
> 
> Juergen, the diff returns wrong name of the function silently. I don't
> think it is 'completely fine', I would rather call it a bug, or
> non-documented limitation, but definitely not a feature. As I wrote
> previously, I played with -p a little and observed more similar issues
> with it. The reason is that the next diff code (see below) tries to
> match function names using simple regexp for a line, assuming that
> line with function name cannot start with a 'non-alpha' characters. So
> even adding one more space before a line with function name breaks it.

And there's not supposed to be any blank ahead of a function's
heading.

> diffutils-3.7/src/diff.c:462
>      case 'p':
>        show_c_function = true;
>        add_regexp (&function_regexp_list, "^[[:alpha:]$_]");
> 
> It is probably could be improved by adding 'no : symbol at the end of
> the line' logic to this regexp. It will resolve the issue with labels,
> but will add one more bug (or limitation) about using ":" in such
> lines.
> 
> The issue is more general here. The diff tool should not parse any
> programming languages, it should just compare the strings.

"The strings" being which ones? The limitations (if you want to call
them such) of "diff -p" are, I think, well understood. And there's
no "parsing" here at all - the regexp simply checks the first
character. The time when you wanted to make it skip labels would be
where it would become more like "parsing".

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Jan Beulich]

On 18.07.2019 16:43, Tamas K Lengyel wrote:
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -60,8 +60,8 @@ 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 while-part of do/while loops.  This is unlike the Linux coding style
> +and unlike K&R.  do/while loops are an exception. e.g.:
>   
>   if ( condition )
>   {
> @@ -77,7 +77,8 @@ while ( condition )
>       /* Do stuff. */
>   }
>   
> -do {
> +do
> +{
>       /* Do stuff. */
>   } while ( condition );

I disagree with this change: There's a large number of (correct as
per the text prior to your change) instances, and since there's
nothing affecting the length of such lines avoiding the extra line
is quite desirable imo.

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

On Thu, Jul 18, 2019 at 9:14 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> On 18.07.2019 16:43, Tamas K Lengyel wrote:
> > --- a/CODING_STYLE
> > +++ b/CODING_STYLE
> > @@ -60,8 +60,8 @@ 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 while-part of do/while loops.  This is unlike the Linux coding style
> > +and unlike K&R.  do/while loops are an exception. e.g.:
> >
> >   if ( condition )
> >   {
> > @@ -77,7 +77,8 @@ while ( condition )
> >       /* Do stuff. */
> >   }
> >
> > -do {
> > +do
> > +{
> >       /* Do stuff. */
> >   } while ( condition );
>
> I disagree with this change: There's a large number of (correct as
> per the text prior to your change) instances, and since there's
> nothing affecting the length of such lines avoiding the extra line
> is quite desirable imo.
>

I understand. However, the upside of the change would be that we can
automate it. I couldn't find a way to override the bsd format to
incorporate this exception. So in my book this change is still well
worth it.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Viktor Mitin]

Hi All,

On Thu, Jul 18, 2019 at 6:16 PM Jan Beulich <JBeulich@suse.com> wrote:
>
> On 18.07.2019 16:43, Tamas K Lengyel wrote:
> > --- a/CODING_STYLE
> > +++ b/CODING_STYLE
> > @@ -60,8 +60,8 @@ 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 while-part of do/while loops.  This is unlike the Linux coding style
> > +and unlike K&R.  do/while loops are an exception. e.g.:
> >
> >   if ( condition )
> >   {
> > @@ -77,7 +77,8 @@ while ( condition )
> >       /* Do stuff. */
> >   }
> >
> > -do {
> > +do
> > +{
> >       /* Do stuff. */
> >   } while ( condition );
>
> I disagree with this change: There's a large number of (correct as
> per the text prior to your change) instances, and since there's
> nothing affecting the length of such lines avoiding the extra line
> is quite desirable imo.
>

Please be aware that 'xen modified' clang-format supports such
'do/while' braces exception.
It has been tested previously and works well.

Thanks

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

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

On Fri, Jul 26, 2019 at 8:42 AM Viktor Mitin <viktor.mitin.19@gmail.com> wrote:
>
> Hi All,
>
> On Thu, Jul 18, 2019 at 6:16 PM Jan Beulich <JBeulich@suse.com> wrote:
> >
> > On 18.07.2019 16:43, Tamas K Lengyel wrote:
> > > --- a/CODING_STYLE
> > > +++ b/CODING_STYLE
> > > @@ -60,8 +60,8 @@ 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 while-part of do/while loops.  This is unlike the Linux coding style
> > > +and unlike K&R.  do/while loops are an exception. e.g.:
> > >
> > >   if ( condition )
> > >   {
> > > @@ -77,7 +77,8 @@ while ( condition )
> > >       /* Do stuff. */
> > >   }
> > >
> > > -do {
> > > +do
> > > +{
> > >       /* Do stuff. */
> > >   } while ( condition );
> >
> > I disagree with this change: There's a large number of (correct as
> > per the text prior to your change) instances, and since there's
> > nothing affecting the length of such lines avoiding the extra line
> > is quite desirable imo.
> >
>
> Please be aware that 'xen modified' clang-format supports such
> 'do/while' braces exception.
> It has been tested previously and works well.
>

Sounds good to me, I would say let's go with that if it reduces churn
in the code.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Andrew Cooper]

On 18/07/2019 15:43, Tamas K Lengyel wrote:
> diff --git a/CODING_STYLE b/CODING_STYLE
> index 6cc5b774cf..0b37f7ae4d 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -60,8 +60,8 @@ 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 while-part of do/while loops.  This is unlike the Linux coding style
> +and unlike K&R.  do/while loops are an exception. e.g.:
>  
>  if ( condition )
>  {
> @@ -77,7 +77,8 @@ while ( condition )
>      /* Do stuff. */
>  }
>  
> -do {
> +do
> +{

I'd happily take this adjustment to Xen's style if it helps us end up
with auto-formatter.

Also, there are a number of files which are technically Linux style, but
have totally diverged from Linux, and would be easier to move to Xen style.

Do you have an updated .astylerc based on Julien's observations?

ISTR someone saying that the maintainer of astyle was very happy taking
additions, if we need to go along that route.

~Andrew

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

On Thu, Jul 18, 2019 at 11:59 AM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> On 18/07/2019 15:43, Tamas K Lengyel wrote:
> > diff --git a/CODING_STYLE b/CODING_STYLE
> > index 6cc5b774cf..0b37f7ae4d 100644
> > --- a/CODING_STYLE
> > +++ b/CODING_STYLE
> > @@ -60,8 +60,8 @@ 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 while-part of do/while loops.  This is unlike the Linux coding style
> > +and unlike K&R.  do/while loops are an exception. e.g.:
> >
> >  if ( condition )
> >  {
> > @@ -77,7 +77,8 @@ while ( condition )
> >      /* Do stuff. */
> >  }
> >
> > -do {
> > +do
> > +{
>
> I'd happily take this adjustment to Xen's style if it helps us end up
> with auto-formatter.

Yay!

>
> Also, there are a number of files which are technically Linux style, but
> have totally diverged from Linux, and would be easier to move to Xen style.
>
> Do you have an updated .astylerc based on Julien's observations?

Yes, this is it:

style=bsd
suffix=none
align-pointer=name
align-reference=name
indent=spaces=4
max-code-length=80
min-conditional-indent=0
max-continuation-indent=78
attach-closing-while
remove-braces
break-one-line-headers
pad-comma
pad-header

With this it's down to 860 files that are formatted.

>
> ISTR someone saying that the maintainer of astyle was very happy taking
> additions, if we need to go along that route.

Right, adding a "xen" style template would be a possible route, but
IMHO we are not that far off from the bsd style to warrant it.

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

Hi,

On 18/07/2019 19:34, Tamas K Lengyel wrote:
> On Thu, Jul 18, 2019 at 11:59 AM Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>>
>> On 18/07/2019 15:43, Tamas K Lengyel wrote:
>>> diff --git a/CODING_STYLE b/CODING_STYLE
>>> index 6cc5b774cf..0b37f7ae4d 100644
>>> --- a/CODING_STYLE
>>> +++ b/CODING_STYLE
>>> @@ -60,8 +60,8 @@ 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 while-part of do/while loops.  This is unlike the Linux coding style
>>> +and unlike K&R.  do/while loops are an exception. e.g.:
>>>
>>>   if ( condition )
>>>   {
>>> @@ -77,7 +77,8 @@ while ( condition )
>>>       /* Do stuff. */
>>>   }
>>>
>>> -do {
>>> +do
>>> +{
>>
>> I'd happily take this adjustment to Xen's style if it helps us end up
>> with auto-formatter.
> 
> Yay!
> 
>>
>> Also, there are a number of files which are technically Linux style, but
>> have totally diverged from Linux, and would be easier to move to Xen style.
>>
>> Do you have an updated .astylerc based on Julien's observations?
> 
> Yes, this is it:
> 
> style=bsd
> suffix=none
> align-pointer=name
> align-reference=name
> indent=spaces=4
> max-code-length=80
> min-conditional-indent=0
> max-continuation-indent=78
> attach-closing-while
> remove-braces
> break-one-line-headers
> pad-comma
> pad-header

Unfortunately this style does not work with the astyle provided by Debian Stretch:

42sh> astyle xen/arch/arm/setup.c
Invalid option file options:
max-continuation-indent=78
attach-closing-while
remove-braces
For help on options type 'astyle -h'

Artistic Style has terminated

Also, I needed to copy the .astylerc in $(HOME) in order to use the style. It is 
probably worth considering implementing a wrapper that set 
ARTISTIC_STYLE_OPTIONS and call astyle to keep everthing in Xen internals.

> 
> With this it's down to 860 files that are formatted.

Do you mind providing the new diff?

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

On Fri, Jul 19, 2019 at 3:03 AM Julien Grall <julien.grall@arm.com> wrote:
>
> Hi,
>
> On 18/07/2019 19:34, Tamas K Lengyel wrote:
> > On Thu, Jul 18, 2019 at 11:59 AM Andrew Cooper
> > <andrew.cooper3@citrix.com> wrote:
> >>
> >> On 18/07/2019 15:43, Tamas K Lengyel wrote:
> >>> diff --git a/CODING_STYLE b/CODING_STYLE
> >>> index 6cc5b774cf..0b37f7ae4d 100644
> >>> --- a/CODING_STYLE
> >>> +++ b/CODING_STYLE
> >>> @@ -60,8 +60,8 @@ 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 while-part of do/while loops.  This is unlike the Linux coding style
> >>> +and unlike K&R.  do/while loops are an exception. e.g.:
> >>>
> >>>   if ( condition )
> >>>   {
> >>> @@ -77,7 +77,8 @@ while ( condition )
> >>>       /* Do stuff. */
> >>>   }
> >>>
> >>> -do {
> >>> +do
> >>> +{
> >>
> >> I'd happily take this adjustment to Xen's style if it helps us end up
> >> with auto-formatter.
> >
> > Yay!
> >
> >>
> >> Also, there are a number of files which are technically Linux style, but
> >> have totally diverged from Linux, and would be easier to move to Xen style.
> >>
> >> Do you have an updated .astylerc based on Julien's observations?
> >
> > Yes, this is it:
> >
> > style=bsd
> > suffix=none
> > align-pointer=name
> > align-reference=name
> > indent=spaces=4
> > max-code-length=80
> > min-conditional-indent=0
> > max-continuation-indent=78
> > attach-closing-while
> > remove-braces
> > break-one-line-headers
> > pad-comma
> > pad-header
>
> Unfortunately this style does not work with the astyle provided by Debian Stretch:
>
> 42sh> astyle xen/arch/arm/setup.c
> Invalid option file options:
> max-continuation-indent=78
> attach-closing-while
> remove-braces
> For help on options type 'astyle -h'
>
> Artistic Style has terminated

I'm already on buster and it works fine. Perhaps we'll need to specify
a minimum version of astyle.

>
> Also, I needed to copy the .astylerc in $(HOME) in order to use the style. It is
> probably worth considering implementing a wrapper that set
> ARTISTIC_STYLE_OPTIONS and call astyle to keep everthing in Xen internals.

You don't have to copy to to $(HOME), as I point out by the addition
to the CODING_STYLE this works from the Xen root folder:

export ARTISTIC_STYLE_OPTIONS=".astylerc"
astyle <source or header file>

>
> >
> > With this it's down to 860 files that are formatted.
>
> Do you mind providing the new diff?

I've updated the same gist with the new diff, the url is the same:
https://gist.github.com/tklengyel/c5cac14a0d57f119dd7747a1be6fb260

Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

Hi Tamas,

On 19/07/2019 14:05, Tamas K Lengyel wrote:
> On Fri, Jul 19, 2019 at 3:03 AM Julien Grall <julien.grall@arm.com> wrote:
>>
>> Hi,
>>
>> On 18/07/2019 19:34, Tamas K Lengyel wrote:
>>> On Thu, Jul 18, 2019 at 11:59 AM Andrew Cooper
>>> <andrew.cooper3@citrix.com> wrote:
>>>>
>>>> On 18/07/2019 15:43, Tamas K Lengyel wrote:
>>>>> diff --git a/CODING_STYLE b/CODING_STYLE
>>>>> index 6cc5b774cf..0b37f7ae4d 100644
>>>>> --- a/CODING_STYLE
>>>>> +++ b/CODING_STYLE
>>>>> @@ -60,8 +60,8 @@ 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 while-part of do/while loops.  This is unlike the Linux coding style
>>>>> +and unlike K&R.  do/while loops are an exception. e.g.:
>>>>>
>>>>>    if ( condition )
>>>>>    {
>>>>> @@ -77,7 +77,8 @@ while ( condition )
>>>>>        /* Do stuff. */
>>>>>    }
>>>>>
>>>>> -do {
>>>>> +do
>>>>> +{
>>>>
>>>> I'd happily take this adjustment to Xen's style if it helps us end up
>>>> with auto-formatter.
>>>
>>> Yay!
>>>
>>>>
>>>> Also, there are a number of files which are technically Linux style, but
>>>> have totally diverged from Linux, and would be easier to move to Xen style.
>>>>
>>>> Do you have an updated .astylerc based on Julien's observations?
>>>
>>> Yes, this is it:
>>>
>>> style=bsd
>>> suffix=none
>>> align-pointer=name
>>> align-reference=name
>>> indent=spaces=4
>>> max-code-length=80
>>> min-conditional-indent=0
>>> max-continuation-indent=78
>>> attach-closing-while
>>> remove-braces
>>> break-one-line-headers
>>> pad-comma
>>> pad-header
>>
>> Unfortunately this style does not work with the astyle provided by Debian Stretch:
>>
>> 42sh> astyle xen/arch/arm/setup.c
>> Invalid option file options:
>> max-continuation-indent=78
>> attach-closing-while
>> remove-braces
>> For help on options type 'astyle -h'
>>
>> Artistic Style has terminated
> 
> I'm already on buster and it works fine. Perhaps we'll need to specify
> a minimum version of astyle.

That would be good.

> 
>>
>> Also, I needed to copy the .astylerc in $(HOME) in order to use the style. It is
>> probably worth considering implementing a wrapper that set
>> ARTISTIC_STYLE_OPTIONS and call astyle to keep everthing in Xen internals.
> 
> You don't have to copy to to $(HOME), as I point out by the addition
> to the CODING_STYLE this works from the Xen root folder:
> 
> export ARTISTIC_STYLE_OPTIONS=".astylerc"
> astyle <source or header file>

I misread your first e-mail. Sorry for the noise.

> 
>>
>>>
>>> With this it's down to 860 files that are formatted.
>>
>> Do you mind providing the new diff?
> 
> I've updated the same gist with the new diff, the url is the same:
> https://gist.github.com/tklengyel/c5cac14a0d57f119dd7747a1be6fb260

Do you mind to create a new gist everytime? This would help to see the 
difference before and after at least until I build a new version of astyle :).

Cheers,

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Tamas K Lengyel]

On Fri, Jul 19, 2019 at 7:34 AM Julien Grall <julien.grall@arm.com> wrote:
>
> Hi Tamas,
>
> On 19/07/2019 14:05, Tamas K Lengyel wrote:
> > On Fri, Jul 19, 2019 at 3:03 AM Julien Grall <julien.grall@arm.com> wrote:
> >>
> >> Hi,
> >>
> >> On 18/07/2019 19:34, Tamas K Lengyel wrote:
> >>> On Thu, Jul 18, 2019 at 11:59 AM Andrew Cooper
> >>> <andrew.cooper3@citrix.com> wrote:
> >>>>
> >>>> On 18/07/2019 15:43, Tamas K Lengyel wrote:
> >>>>> diff --git a/CODING_STYLE b/CODING_STYLE
> >>>>> index 6cc5b774cf..0b37f7ae4d 100644
> >>>>> --- a/CODING_STYLE
> >>>>> +++ b/CODING_STYLE
> >>>>> @@ -60,8 +60,8 @@ 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 while-part of do/while loops.  This is unlike the Linux coding style
> >>>>> +and unlike K&R.  do/while loops are an exception. e.g.:
> >>>>>
> >>>>>    if ( condition )
> >>>>>    {
> >>>>> @@ -77,7 +77,8 @@ while ( condition )
> >>>>>        /* Do stuff. */
> >>>>>    }
> >>>>>
> >>>>> -do {
> >>>>> +do
> >>>>> +{
> >>>>
> >>>> I'd happily take this adjustment to Xen's style if it helps us end up
> >>>> with auto-formatter.
> >>>
> >>> Yay!
> >>>
> >>>>
> >>>> Also, there are a number of files which are technically Linux style, but
> >>>> have totally diverged from Linux, and would be easier to move to Xen style.
> >>>>
> >>>> Do you have an updated .astylerc based on Julien's observations?
> >>>
> >>> Yes, this is it:
> >>>
> >>> style=bsd
> >>> suffix=none
> >>> align-pointer=name
> >>> align-reference=name
> >>> indent=spaces=4
> >>> max-code-length=80
> >>> min-conditional-indent=0
> >>> max-continuation-indent=78
> >>> attach-closing-while
> >>> remove-braces
> >>> break-one-line-headers
> >>> pad-comma
> >>> pad-header
> >>
> >> Unfortunately this style does not work with the astyle provided by Debian Stretch:
> >>
> >> 42sh> astyle xen/arch/arm/setup.c
> >> Invalid option file options:
> >> max-continuation-indent=78
> >> attach-closing-while
> >> remove-braces
> >> For help on options type 'astyle -h'
> >>
> >> Artistic Style has terminated
> >
> > I'm already on buster and it works fine. Perhaps we'll need to specify
> > a minimum version of astyle.
>
> That would be good.
>
> >
> >>
> >> Also, I needed to copy the .astylerc in $(HOME) in order to use the style. It is
> >> probably worth considering implementing a wrapper that set
> >> ARTISTIC_STYLE_OPTIONS and call astyle to keep everthing in Xen internals.
> >
> > You don't have to copy to to $(HOME), as I point out by the addition
> > to the CODING_STYLE this works from the Xen root folder:
> >
> > export ARTISTIC_STYLE_OPTIONS=".astylerc"
> > astyle <source or header file>
>
> I misread your first e-mail. Sorry for the noise.
>
> >
> >>
> >>>
> >>> With this it's down to 860 files that are formatted.
> >>
> >> Do you mind providing the new diff?
> >
> > I've updated the same gist with the new diff, the url is the same:
> > https://gist.github.com/tklengyel/c5cac14a0d57f119dd7747a1be6fb260
>
> Do you mind to create a new gist everytime? This would help to see the
> difference before and after at least until I build a new version of astyle :).

Sure, no problem. Also a tip: just enable the buster repo and do
"apt-get -t buster install astyle" ;)

Cheers,
Tamas

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Rich Persaud]

> On Jul 18, 2019, at 10:43, Tamas K Lengyel <tamas@tklengyel.com> wrote:
> 
> Using astyle (http://astyle.sourceforge.net) can greatly reduce the overhead of
> manually checking and applying style-fixes to source-code. The included
> .astylerc is the closest approximation of the established Xen style (including
> styles not formally spelled out by CODING_STYLE but commonly requested).
> 
> Checking the comment styles are not included in the automation.
> 
> Incorporating Xen's exception to the do-while style is only partially possible,
> thus a change is proposed to the CODING_STYLE of moving the brace from "do {"
> to the next line.
> 
> Most of Xen's code-base is non-conforming at the moment: 289 files pass
> unchanged, 876 have some style issues.
> 
> Ideally we can slowly migrate the entire code-base to be conforming, thus
> eliminating the need of discussing and enforcing style issues manually on the
> mailinglist.

Thanks for taking the lead on this, Tamas.  New Xen contributors are unlikely to be aware of the style ambiguities discussed in this thread. A frequent topic on Xen monthly calls is the lack of time to perform patch reviews.  Automated style checking will increase the S/N ratio of xen-devel, reviewer efficiency, patch quality from new contributors, and style consistency across Xen trees.  This automation will complement upcoming static analysis of Xen for functional safety compliance.

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Viktor Mitin]

Hi All,

On Thu, Jul 18, 2019 at 5:45 PM Tamas K Lengyel <tamas@tklengyel.com> wrote:

> Checking the comment styles are not included in the automation.

The same about clang-format. Checking the comment styles is not supported.
It seems this is not code format checker task to parse and modify the
code comments...

Thanks

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Julien Grall]

On 26/07/2019 16:48, Viktor Mitin wrote:
> Hi All,
> 
> On Thu, Jul 18, 2019 at 5:45 PM Tamas K Lengyel <tamas@tklengyel.com> wrote:
> 
>> Checking the comment styles are not included in the automation.
> 
> The same about clang-format. Checking the comment styles is not supported.
> It seems this is not code format checker task to parse and modify the
> code comments...

Are you sure about your statement? Looking at the diff you provided [1], 
clang-format is clearly trying to alter the comments.

But comments flow are equally important as the code. This is part of the coding 
style afterall.

[1] 
https://raw.githubusercontent.com/viktor-mitin/xen-clang-format-example/master/0001-clang-format-checkpatch-output-example.patch

> 
> Thanks
> 

-- 
Julien Grall

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

Re: [Xen-devel] [RFC] xen: Add .astylerc for automated style-formatting

[Viktor Mitin]

On Fri, Jul 26, 2019 at 6:54 PM Julien Grall <julien.grall@arm.com> wrote:
>
>
>
> On 26/07/2019 16:48, Viktor Mitin wrote:
> > Hi All,
> >
> > On Thu, Jul 18, 2019 at 5:45 PM Tamas K Lengyel <tamas@tklengyel.com> wrote:
> >
> >> Checking the comment styles are not included in the automation.
> >
> > The same about clang-format. Checking the comment styles is not supported.
> > It seems this is not code format checker task to parse and modify the
> > code comments...
>
> Are you sure about your statement? Looking at the diff you provided [1],
> clang-format is clearly trying to alter the comments.
>
> But comments flow are equally important as the code. This is part of the coding
> style afterall.
>
> [1]
> https://raw.githubusercontent.com/viktor-mitin/xen-clang-format-example/master/0001-clang-format-checkpatch-output-example.patch

Clang-format supports only basic indentation of C89 comments.

--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -51,15 +51,15 @@ void arch_livepatch_apply(struct livepatch_func *func)
         *(new_ptr + i) = insn;

     /*
-    * When we upload the payload, it will go through the data cache
-    * (the region is cacheable). Until the data cache is cleaned, the data
-    * may not reach the memory. And in the case the data and instruction cache
-    * are separated, we may read invalid instruction from the memory because
-    * the data cache have not yet synced with the memory. Hence sync it.
-    */
+     * When we upload the payload, it will go through the data cache
+     * (the region is cacheable). Until the data cache is cleaned, the data
+     * may not reach the memory. And in the case the data and instruction cache
+     * are separated, we may read invalid instruction from the memory because
+     * the data cache have not yet synced with the memory. Hence sync it.
+     */

So it addresses Xen Coding Style requirements partially:

From CODING_STYLE:
"
Multi-line comment blocks should start and end with comment markers on
separate lines and each line should begin with a leading '*'.

/*
 * Example, multi-line comment block.
 *
 * Note beginning and end markers on separate lines and leading '*'.
 */
"

However, the next comments rules are not supported for now and can be
added into implementation later.

From CODING_STYLE:
"
Only C style /* ... */ comments are to be used.  C++ style // comments
should not be used.  Multi-word comments should begin with a capital
letter.  Comments containing a single sentence may end with a full
stop; comments containing several sentences must have a full stop
after each sentence.
"

BTW, is there an actual reason not to support C99 comments ( // ) ?

Thanks

>
> >
> > Thanks
> >
>
> --
> Julien Grall

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