Re: [Xen-devel] Xen Coding style and clang-format

2019-08-01 Thread Viktor Mitin 
Hi Tamas,

On Thu, Aug 1, 2019 at 6:58 PM Tamas K Lengyel
 wrote:
>
> On Thu, Aug 1, 2019 at 4:06 AM Viktor Mitin  wrote:
> >
> > On Wed, Jul 31, 2019 at 8:05 PM Viktor Mitin  
> > wrote:
> > >
> > > On Wed, Jul 31, 2019 at 7:27 PM Lars Kurth  
> > > wrote:
> > >
> > > > Viktor: thank you for spending time on this
> > > >
> > > > I added an item to community call tomorrow and CC'ed you in the invite. 
> > > > So I think what we need to do is figure out a way on how to make the 
> > > > coding standard enforceable by a coding standard checker such as 
> > > > proposed here. AFAICT
> > > > * It seems there are some undocumented coding standard rules, which are 
> > > > essentially causing problems with the tool
> > > > * In addition, the fact that the LLVM coding style is the baseline for 
> > > > the checks may also create some problems with false standard violations
> > > >
> > > > My instinct would be to try and document any undocumented rules on a 
> > > > scratch space (e.g. google doc), look at differences between Xen and 
> > > > LLVM formatting style and then make decisions using a voting mechanism 
> > > > to avoid bike-shedding. In some cases discussion may be necessary though
> > > >
> > > > It would be good if you could attend, but I think we can do without 
> > > > you, if needed
> > > >
> > >
> > > Lars, thank you for the invitation. I will try to join the call.
> > > Seems the topic is not a simple one, there are a lot of things to discuss 
> > > it.
> >
> > Please be aware that the repo with xen clang-format has been created
> > under the next link (branch xen-clang-format):
> > https://github.com/xen-troops/llvm-project/tree/xen-clang-format
> >
> > The next script can be used as an example of how to build clang-format:
> > https://github.com/viktor-mitin/xen-clang-format-example/blob/master/build_clang_format.sh
>
> I had a chance to give it a shot right now and running it on the
> hypervisor code results in 1073 out of 1165 files being changed by it.
> Here is the gist of the diff:
>
> https://gist.github.com/tklengyel/bc4a86e0f20b7c50c730c1b9429d4e2c

Thank you for checking the tool. There are many files changed because
it needs to investigate how to implement 'lazy' mode logic in it.
As we discussed with Julien, it should not try to pack everything to
fill a line, etc. Once it is done, many of such 'false-positives'
disappear.
In any case, thank you for your input about it.

Thanks

> Cheers,
> Tamas

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

Re: [Xen-devel] Xen Coding style and clang-format

2019-08-01 Thread Tamas K Lengyel 
On Thu, Aug 1, 2019 at 4:06 AM Viktor Mitin  wrote:
>
> On Wed, Jul 31, 2019 at 8:05 PM Viktor Mitin  
> wrote:
> >
> > On Wed, Jul 31, 2019 at 7:27 PM Lars Kurth  wrote:
> >
> > > Viktor: thank you for spending time on this
> > >
> > > I added an item to community call tomorrow and CC'ed you in the invite. 
> > > So I think what we need to do is figure out a way on how to make the 
> > > coding standard enforceable by a coding standard checker such as proposed 
> > > here. AFAICT
> > > * It seems there are some undocumented coding standard rules, which are 
> > > essentially causing problems with the tool
> > > * In addition, the fact that the LLVM coding style is the baseline for 
> > > the checks may also create some problems with false standard violations
> > >
> > > My instinct would be to try and document any undocumented rules on a 
> > > scratch space (e.g. google doc), look at differences between Xen and LLVM 
> > > formatting style and then make decisions using a voting mechanism to 
> > > avoid bike-shedding. In some cases discussion may be necessary though
> > >
> > > It would be good if you could attend, but I think we can do without you, 
> > > if needed
> > >
> >
> > Lars, thank you for the invitation. I will try to join the call.
> > Seems the topic is not a simple one, there are a lot of things to discuss 
> > it.
>
> Please be aware that the repo with xen clang-format has been created
> under the next link (branch xen-clang-format):
> https://github.com/xen-troops/llvm-project/tree/xen-clang-format
>
> The next script can be used as an example of how to build clang-format:
> https://github.com/viktor-mitin/xen-clang-format-example/blob/master/build_clang_format.sh

I had a chance to give it a shot right now and running it on the
hypervisor code results in 1073 out of 1165 files being changed by it.
Here is the gist of the diff:

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

Cheers,
Tamas

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

Re: [Xen-devel] Xen Coding style and clang-format

2019-08-01 Thread Viktor Mitin 
On Wed, Jul 31, 2019 at 8:05 PM Viktor Mitin  wrote:
>
> On Wed, Jul 31, 2019 at 7:27 PM Lars Kurth  wrote:
>
> > Viktor: thank you for spending time on this
> >
> > I added an item to community call tomorrow and CC'ed you in the invite. So 
> > I think what we need to do is figure out a way on how to make the coding 
> > standard enforceable by a coding standard checker such as proposed here. 
> > AFAICT
> > * It seems there are some undocumented coding standard rules, which are 
> > essentially causing problems with the tool
> > * In addition, the fact that the LLVM coding style is the baseline for the 
> > checks may also create some problems with false standard violations
> >
> > My instinct would be to try and document any undocumented rules on a 
> > scratch space (e.g. google doc), look at differences between Xen and LLVM 
> > formatting style and then make decisions using a voting mechanism to avoid 
> > bike-shedding. In some cases discussion may be necessary though
> >
> > It would be good if you could attend, but I think we can do without you, if 
> > needed
> >
>
> Lars, thank you for the invitation. I will try to join the call.
> Seems the topic is not a simple one, there are a lot of things to discuss it.

Please be aware that the repo with xen clang-format has been created
under the next link (branch xen-clang-format):
https://github.com/xen-troops/llvm-project/tree/xen-clang-format

The next script can be used as an example of how to build clang-format:
https://github.com/viktor-mitin/xen-clang-format-example/blob/master/build_clang_format.sh

I've added a couple more updates discussed these days:
- Max line length from 80 to 79 chars;
- Set BreakStringLiterals to false.

Please see the updated xen-clang-format summary below:

Summary of the changes:
- Added 3 new formatting styles to cover all the cases mentioned in
Xen coding style document: Xen, Libxl, Linux;
- Added list of the files and corresponding style name mappings;
- Added indentation according to Xen coding style;
- Added white space formatting according to Xen coding style;
- Added bracing support exception for do/while loops;

Added to clang-format, however, probably this logic should be moved to
python part (see known clang-format limitations above):
- Braces should be omitted for blocks with a single statement. Note:
these braces will be required by MISRA, for example, so it is probably
worth adding such a requirement to the coding style.
- Comments format requirements. Note: //-style comments are defined in
C99 as well, and not just in the case of C++. C99 standard is 20-years
old…

To be added:
- Emacs local variables. Open points: Why to keep emacs local
variables in Xen code? What about other editors' comments (vim)?
- Warning to stderr in the case when ‘unfixable’ line/s detected.

To be fixed:
- Disable // comments;

Thanks

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

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-31 Thread Viktor Mitin 
On Wed, Jul 31, 2019 at 7:27 PM Lars Kurth  wrote:

> Viktor: thank you for spending time on this
>
> I added an item to community call tomorrow and CC'ed you in the invite. So I 
> think what we need to do is figure out a way on how to make the coding 
> standard enforceable by a coding standard checker such as proposed here. 
> AFAICT
> * It seems there are some undocumented coding standard rules, which are 
> essentially causing problems with the tool
> * In addition, the fact that the LLVM coding style is the baseline for the 
> checks may also create some problems with false standard violations
>
> My instinct would be to try and document any undocumented rules on a scratch 
> space (e.g. google doc), look at differences between Xen and LLVM formatting 
> style and then make decisions using a voting mechanism to avoid 
> bike-shedding. In some cases discussion may be necessary though
>
> It would be good if you could attend, but I think we can do without you, if 
> needed
>

Lars, thank you for the invitation. I will try to join the call.
Seems the topic is not a simple one, there are a lot of things to discuss it.

Thanks

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

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-31 Thread Lars Kurth 


> On 31 Jul 2019, at 12:43, Viktor Mitin  wrote:
> 
> On Wed, Jul 31, 2019 at 2:25 PM Julien Grall  > wrote:
>> 
>> 
>> 
>> On 31/07/2019 12:16, Viktor Mitin wrote:
>>> On Mon, Jul 29, 2019 at 3:35 PM Julien Grall  wrote:
 On 7/29/19 1:21 PM, Viktor Mitin wrote:
> On Mon, Jul 29, 2019 at 1:49 PM Julien Grall  wrote:
>> On 7/29/19 10:13 AM, Viktor Mitin wrote:
>>> On Fri, Jul 26, 2019 at 3:50 PM Julien Grall  
>>> wrote:
 
 *
 
 -/* See linux
 Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
 +/* See linux
 + * 
 Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
 
 Multi-lines comment on Xen are using
 /*
 * Foo
 * Bar
 */
>>> 
>>> See my comment about clang-format supports only comments indentation 
>>> for now.
>> 
>> I saw it and I will reply here for simplicity. Having a automatic
>> checker that will do the wrong things is not ideal.
>> 
>> Imagine we decide to use this checker as a part of the commit process.
>> This means that the code will be modified to checker coding style and
>> not Xen one.
> 
> Well, you are right. Even more, unfortunately, there is no such coding
> style as 'bsd' in clang-format.
> So 'xen' clang-format style is based on the 'llvm' style.
 
 Do you have a pointer to the LLVM style?
>>> 
>>> Sure, see the next links:
>>> https://github.com/viktor-mitin/xen-clang-format-example/blob/master/.clang-format_llvm
>>> https://github.com/viktor-mitin/xen-clang-format-example/blob/master/.clang-format_xen
>> 
>> That's clang-format configuration not a write-up easily readable by human. 
>> It is
>> also does not say what will happen for the rest of the things not configured 
>> (if
>> there are any).
> 
> Here is Clang-Format Style Options documentation:
> https://clang.llvm.org/docs/ClangFormatStyleOptions.html 
> 
> 
> And LLVM Coding Standards documetation:
> https://llvm.org/docs/CodingStandards.html#introduction 
> 
> 
> Unfortunately, it seems it does not answer "what will happen for the
> rest of the things not configured (if there are any)"...
> 
> 
>> 
>>> 
 
 As I pointed out in a different thread, it woudl be easier to start from
 an existing coding style (LLVM, BSD...) and decide whether you want to
 fully adopt it or make changes.
 
 So someone needs to be pick one and look at the difference in style with
 Xen. It seems you already done that job as you tweak it for Xen. Do you
 have a write-up of the differences?
>>> 
>>> Yes, it is done exactly this way you mentioned. New 'xen' format style
>>> is based on 'llvm'.
>> 
>> Can you give a link to this write-up in a human readable way?
> 
> The summary I wrote in the original mail in this thread describes what
> was done based on 'llvm' coding style of clang-format.
> I'm putting it here with update: added BreakStringLiterals thing to be fixed.
> 
> Summary of the changes:
> - Added 3 new formatting styles to cover all the cases mentioned in
> Xen coding style document: Xen, Libxl, Linux;
> - Added list of the files and corresponding style name mappings;
> - Added indentation according to Xen coding style;
> - Added white space formatting according to Xen coding style;
> - Added bracing support exception for do/while loops;
> 
> Added to clang-format, however, probably this logic should be moved to
> python part (see known clang-format limitations above):
> - Braces should be omitted for blocks with a single statement. Note:
> these braces will be required by MISRA, for example, so it is probably
> worth adding such a requirement to the coding style.
> - Comments format requirements. Note: //-style comments are defined in
> C99 as well, and not just in the case of C++. C99 standard is 20-years
> old…
> 
> To be added:
> - Emacs local variables. Open points: Why to keep emacs local
> variables in Xen code? What about other editors' comments (vim)?
> - Warning to stderr in the case when ‘unfixable’ line/s detected.
> 
> To be fixed:
> - Max line length from 80 to 79 chars;
> - Disable // comments;
> - Set BreakStringLiterals to false (not explicitly covered by Xen
> coding style document for now)
> 
>> 
>>> 
 
 I am not sure why clang-format decided to format like that. Do you 
 know why?
>>> 
>>> The reason is that there are two strings in one line. It would not
>>> change it if it were
>>> not "arm,psci-1.0""\0", but "arm,psci-1.0\0".
>> 
>> I would like to see the exact part of the clang-format coding style
>> documentation that mention this requirements... The more that in an
>> example above (copied below for simplicity), there are two

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-31 Thread Viktor Mitin 
On Wed, Jul 31, 2019 at 2:25 PM Julien Grall  wrote:
>
>
>
> On 31/07/2019 12:16, Viktor Mitin wrote:
> > On Mon, Jul 29, 2019 at 3:35 PM Julien Grall  wrote:
> >> On 7/29/19 1:21 PM, Viktor Mitin wrote:
> >>> On Mon, Jul 29, 2019 at 1:49 PM Julien Grall  wrote:
>  On 7/29/19 10:13 AM, Viktor Mitin wrote:
> > On Fri, Jul 26, 2019 at 3:50 PM Julien Grall  
> > wrote:
> >>
> >> *
> >>
> >> -/* See linux
> >> Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
> >> +/* See linux
> >> + * 
> >> Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
> >>
> >> Multi-lines comment on Xen are using
> >> /*
> >>  * Foo
> >>  * Bar
> >>  */
> >
> > See my comment about clang-format supports only comments indentation 
> > for now.
> 
>  I saw it and I will reply here for simplicity. Having a automatic
>  checker that will do the wrong things is not ideal.
> 
>  Imagine we decide to use this checker as a part of the commit process.
>  This means that the code will be modified to checker coding style and
>  not Xen one.
> >>>
> >>> Well, you are right. Even more, unfortunately, there is no such coding
> >>> style as 'bsd' in clang-format.
> >>> So 'xen' clang-format style is based on the 'llvm' style.
> >>
> >> Do you have a pointer to the LLVM style?
> >
> > Sure, see the next links:
> > https://github.com/viktor-mitin/xen-clang-format-example/blob/master/.clang-format_llvm
> > https://github.com/viktor-mitin/xen-clang-format-example/blob/master/.clang-format_xen
>
> That's clang-format configuration not a write-up easily readable by human. It 
> is
> also does not say what will happen for the rest of the things not configured 
> (if
> there are any).

Here is Clang-Format Style Options documentation:
https://clang.llvm.org/docs/ClangFormatStyleOptions.html

And LLVM Coding Standards documetation:
 https://llvm.org/docs/CodingStandards.html#introduction

Unfortunately, it seems it does not answer "what will happen for the
rest of the things not configured (if there are any)"...


>
> >
> >>
> >> As I pointed out in a different thread, it woudl be easier to start from
> >> an existing coding style (LLVM, BSD...) and decide whether you want to
> >> fully adopt it or make changes.
> >>
> >> So someone needs to be pick one and look at the difference in style with
> >> Xen. It seems you already done that job as you tweak it for Xen. Do you
> >> have a write-up of the differences?
> >
> > Yes, it is done exactly this way you mentioned. New 'xen' format style
> > is based on 'llvm'.
>
> Can you give a link to this write-up in a human readable way?

The summary I wrote in the original mail in this thread describes what
was done based on 'llvm' coding style of clang-format.
I'm putting it here with update: added BreakStringLiterals thing to be fixed.

Summary of the changes:
- Added 3 new formatting styles to cover all the cases mentioned in
Xen coding style document: Xen, Libxl, Linux;
- Added list of the files and corresponding style name mappings;
- Added indentation according to Xen coding style;
- Added white space formatting according to Xen coding style;
- Added bracing support exception for do/while loops;

Added to clang-format, however, probably this logic should be moved to
python part (see known clang-format limitations above):
- Braces should be omitted for blocks with a single statement. Note:
these braces will be required by MISRA, for example, so it is probably
worth adding such a requirement to the coding style.
- Comments format requirements. Note: //-style comments are defined in
C99 as well, and not just in the case of C++. C99 standard is 20-years
old…

To be added:
- Emacs local variables. Open points: Why to keep emacs local
variables in Xen code? What about other editors' comments (vim)?
- Warning to stderr in the case when ‘unfixable’ line/s detected.

To be fixed:
- Max line length from 80 to 79 chars;
- Disable // comments;
- Set BreakStringLiterals to false (not explicitly covered by Xen
coding style document for now)

>
> >
> >>
> >> I am not sure why clang-format decided to format like that. Do you 
> >> know why?
> >
> > The reason is that there are two strings in one line. It would not
> > change it if it were
> > not "arm,psci-1.0""\0", but "arm,psci-1.0\0".
> 
>  I would like to see the exact part of the clang-format coding style
>  documentation that mention this requirements... The more that in an
>  example above (copied below for simplicity), there are two strings in on
>  line.
> >>>
> >>> The closest found seems BinPackParameters BinPackArguments,
> >>> however, it is about function calls according to manual...
> >>
> >> Above, you mention the work is based on the LLVM coding style. Is there
> >> anything in that coding style about the string?
> >
> > Well, not much. See

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-31 Thread Julien Grall 



On 31/07/2019 12:16, Viktor Mitin wrote:

On Mon, Jul 29, 2019 at 3:35 PM Julien Grall  wrote:

On 7/29/19 1:21 PM, Viktor Mitin wrote:

On Mon, Jul 29, 2019 at 1:49 PM Julien Grall  wrote:

On 7/29/19 10:13 AM, Viktor Mitin wrote:

On Fri, Jul 26, 2019 at 3:50 PM Julien Grall  wrote:


*

-/* See linux
Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
+/* See linux
+ * Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */

Multi-lines comment on Xen are using
/*
 * Foo
 * Bar
 */


See my comment about clang-format supports only comments indentation for now.


I saw it and I will reply here for simplicity. Having a automatic
checker that will do the wrong things is not ideal.

Imagine we decide to use this checker as a part of the commit process.
This means that the code will be modified to checker coding style and
not Xen one.


Well, you are right. Even more, unfortunately, there is no such coding
style as 'bsd' in clang-format.
So 'xen' clang-format style is based on the 'llvm' style.


Do you have a pointer to the LLVM style?


Sure, see the next links:
https://github.com/viktor-mitin/xen-clang-format-example/blob/master/.clang-format_llvm
https://github.com/viktor-mitin/xen-clang-format-example/blob/master/.clang-format_xen


That's clang-format configuration not a write-up easily readable by human. It is 
also does not say what will happen for the rest of the things not configured (if 
there are any).






As I pointed out in a different thread, it woudl be easier to start from
an existing coding style (LLVM, BSD...) and decide whether you want to
fully adopt it or make changes.

So someone needs to be pick one and look at the difference in style with
Xen. It seems you already done that job as you tweak it for Xen. Do you
have a write-up of the differences?


Yes, it is done exactly this way you mentioned. New 'xen' format style
is based on 'llvm'.


Can you give a link to this write-up in a human readable way?






I am not sure why clang-format decided to format like that. Do you know why?


The reason is that there are two strings in one line. It would not
change it if it were
not "arm,psci-1.0""\0", but "arm,psci-1.0\0".


I would like to see the exact part of the clang-format coding style
documentation that mention this requirements... The more that in an
example above (copied below for simplicity), there are two strings in on
line.


The closest found seems BinPackParameters BinPackArguments,
however, it is about function calls according to manual...


Above, you mention the work is based on the LLVM coding style. Is there
anything in that coding style about the string?


Well, not much. See clang-format configurator mentioned above.
However, there is a useful clang BreakStringLiterals option.
It should be turned off to follow your suggestion not to break string
literal for grep use case.


I am not speaking about clang-format itself but the LLVM coding style. I assume 
there is a human readable coding style for LLVM, right? If so, is there any 
section in it about string?










   >> +D11PRINT("Allocated %#" PRIpaddr "-%#" PRIpaddr






*

-clock_valid = dt_property_read_u32(dev, "clock-frequency",
-   _frequency);
+clock_valid =
+dt_property_read_u32(dev, "clock-frequency", _frequency);

I am not sure why clang-format decide to format like that. The current version
is definitely valid.


The current version is not valid as it takes 81 chars, while 79 is
allowed according to coding style.


Really? I looked at the code and this is 62 characters... It would be 81
characters if "_frequency);" were on the same line. But see how it
is split in 2 lines.


You are right, there are two lines. So it needs to find out how to
configure or implement such a feature to ignore such cases.


What's the LLVM coding style policy about this?


Well, LLVM formats it as shown above. All the other 'native'
clang-format styles behave similarly.


This does not answer to my question. You pointed me how clang-format is 
configured, not how the behavior of clang format for this particular case and 
the developer documentation related to this.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-31 Thread Viktor Mitin 
Hi Julien,

On Mon, Jul 29, 2019 at 3:35 PM Julien Grall  wrote:
>
> Hi Viktor,
>
> On 7/29/19 1:21 PM, Viktor Mitin wrote:
> > On Mon, Jul 29, 2019 at 1:49 PM Julien Grall  wrote:
> >> On 7/29/19 10:13 AM, Viktor Mitin wrote:
> >>> On Fri, Jul 26, 2019 at 3:50 PM Julien Grall  wrote:
> >>
> >>> It is unknown how difficult it would be to implement that with
> >>> clang-format, however, it can be analyzed.
> >>
> >> ...  but the goal of this discussion is to understand the limitations of
> >> Clang-format and whether a Coding Style change may be easier.
> >
> > It is not so easy to do, so it may take a time to investigate every the 
> > case.
>
> There must be a documentation for clang-format to explain the default
> coding style and way to tweak it, right? Could we get a pointer to it?

https://clang.llvm.org/docs/ClangFormat.html
Even more, there is 'clang-format configurator' which allows
experimenting with clang-format options online:
https://zed0.co.uk/clang-format-configurator/

> 
>  *
> 
>  -/* See linux
>  Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
>  +/* See linux
>  + * 
>  Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
> 
>  Multi-lines comment on Xen are using
>  /*
>  * Foo
>  * Bar
>  */
> >>>
> >>> See my comment about clang-format supports only comments indentation for 
> >>> now.
> >>
> >> I saw it and I will reply here for simplicity. Having a automatic
> >> checker that will do the wrong things is not ideal.
> >>
> >> Imagine we decide to use this checker as a part of the commit process.
> >> This means that the code will be modified to checker coding style and
> >> not Xen one.
> >
> > Well, you are right. Even more, unfortunately, there is no such coding
> > style as 'bsd' in clang-format.
> > So 'xen' clang-format style is based on the 'llvm' style.
>
> Do you have a pointer to the LLVM style?

Sure, see the next links:
https://github.com/viktor-mitin/xen-clang-format-example/blob/master/.clang-format_llvm
https://github.com/viktor-mitin/xen-clang-format-example/blob/master/.clang-format_xen

>
> As I pointed out in a different thread, it woudl be easier to start from
> an existing coding style (LLVM, BSD...) and decide whether you want to
> fully adopt it or make changes.
>
> So someone needs to be pick one and look at the difference in style with
> Xen. It seems you already done that job as you tweak it for Xen. Do you
> have a write-up of the differences?

Yes, it is done exactly this way you mentioned. New 'xen' format style
is based on 'llvm'.

>
>  I am not sure why clang-format decided to format like that. Do you know 
>  why?
> >>>
> >>> The reason is that there are two strings in one line. It would not
> >>> change it if it were
> >>> not "arm,psci-1.0""\0", but "arm,psci-1.0\0".
> >>
> >> I would like to see the exact part of the clang-format coding style
> >> documentation that mention this requirements... The more that in an
> >> example above (copied below for simplicity), there are two strings in on
> >> line.
> >
> > The closest found seems BinPackParameters BinPackArguments,
> > however, it is about function calls according to manual...
>
> Above, you mention the work is based on the LLVM coding style. Is there
> anything in that coding style about the string?

Well, not much. See clang-format configurator mentioned above.
However, there is a useful clang BreakStringLiterals option.
It should be turned off to follow your suggestion not to break string
literal for grep use case.

>
> >
> >>
> >>   >> +D11PRINT("Allocated %#" PRIpaddr "-%#" PRIpaddr
> >>
> >>
> >>>
> 
>  *
> 
>  -clock_valid = dt_property_read_u32(dev, "clock-frequency",
>  -   _frequency);
>  +clock_valid =
>  +dt_property_read_u32(dev, "clock-frequency", _frequency);
> 
>  I am not sure why clang-format decide to format like that. The current 
>  version
>  is definitely valid.
> >>>
> >>> The current version is not valid as it takes 81 chars, while 79 is
> >>> allowed according to coding style.
> >>
> >> Really? I looked at the code and this is 62 characters... It would be 81
> >> characters if "_frequency);" were on the same line. But see how it
> >> is split in 2 lines.
> >
> > You are right, there are two lines. So it needs to find out how to
> > configure or implement such a feature to ignore such cases.
>
> What's the LLVM coding style policy about this?

Well, LLVM formats it as shown above. All the other 'native'
clang-format styles behave similarly.

Thanks

>
> Cheers,
>
> --
> Julien Grall

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

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-29 Thread Julien Grall 

Hi Viktor,

On 7/29/19 1:21 PM, Viktor Mitin wrote:

On Mon, Jul 29, 2019 at 1:49 PM Julien Grall  wrote:

On 7/29/19 10:13 AM, Viktor Mitin wrote:

On Fri, Jul 26, 2019 at 3:50 PM Julien Grall  wrote:



It is unknown how difficult it would be to implement that with
clang-format, however, it can be analyzed.


...  but the goal of this discussion is to understand the limitations of
Clang-format and whether a Coding Style change may be easier.


It is not so easy to do, so it may take a time to investigate every the case.


There must be a documentation for clang-format to explain the default 
coding style and way to tweak it, right? Could we get a pointer to it?




*

-/* See linux
Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
+/* See linux
+ * Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */

Multi-lines comment on Xen are using
/*
* Foo
* Bar
*/


See my comment about clang-format supports only comments indentation for now.


I saw it and I will reply here for simplicity. Having a automatic
checker that will do the wrong things is not ideal.

Imagine we decide to use this checker as a part of the commit process.
This means that the code will be modified to checker coding style and
not Xen one.


Well, you are right. Even more, unfortunately, there is no such coding
style as 'bsd' in clang-format.
So 'xen' clang-format style is based on the 'llvm' style.


Do you have a pointer to the LLVM style?

As I pointed out in a different thread, it woudl be easier to start from 
an existing coding style (LLVM, BSD...) and decide whether you want to 
fully adopt it or make changes.


So someone needs to be pick one and look at the difference in style with 
Xen. It seems you already done that job as you tweak it for Xen. Do you 
have a write-up of the differences?



I am not sure why clang-format decided to format like that. Do you know why?


The reason is that there are two strings in one line. It would not
change it if it were
not "arm,psci-1.0""\0", but "arm,psci-1.0\0".


I would like to see the exact part of the clang-format coding style
documentation that mention this requirements... The more that in an
example above (copied below for simplicity), there are two strings in on
line.


The closest found seems BinPackParameters BinPackArguments,
however, it is about function calls according to manual...


Above, you mention the work is based on the LLVM coding style. Is there 
anything in that coding style about the string?






  >> +D11PRINT("Allocated %#" PRIpaddr "-%#" PRIpaddr






*

-clock_valid = dt_property_read_u32(dev, "clock-frequency",
-   _frequency);
+clock_valid =
+dt_property_read_u32(dev, "clock-frequency", _frequency);

I am not sure why clang-format decide to format like that. The current version
is definitely valid.


The current version is not valid as it takes 81 chars, while 79 is
allowed according to coding style.


Really? I looked at the code and this is 62 characters... It would be 81
characters if "_frequency);" were on the same line. But see how it
is split in 2 lines.


You are right, there are two lines. So it needs to find out how to
configure or implement such a feature to ignore such cases.


What's the LLVM coding style policy about this?

Cheers,

--
Julien Grall

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

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-29 Thread Viktor Mitin 
Hi Julien,

On Mon, Jul 29, 2019 at 1:49 PM Julien Grall  wrote:
>
> Hi,
>
> On 7/29/19 10:13 AM, Viktor Mitin wrote:
> > On Fri, Jul 26, 2019 at 3:50 PM Julien Grall  wrote:
> >>
> >> I have already done some testings a couple of weeks ago with the patch 
> >> [1]. I
> >> have sent some comments regarding the change made by the tools that 
> >> require some
> >> attention. It would be good if someone go through them and try to address 
> >> one by
> >> one. For convenience I have replicated my e-mail publicly below.
> >
> >> *** xen/arm/domain_build.c ***
> >>
> >> *
> >>
> >> -D11PRINT("Allocated %#"PRIpaddr"-%#"PRIpaddr" (%ldMB/%ldMB, order 
> >> %d)\n",
> >> - start, start + size,
> >> - 1UL << (order + PAGE_SHIFT - 20),
> >> +D11PRINT("Allocated %#" PRIpaddr "-%#" PRIpaddr
> >> + " (%ldMB/%ldMB, order %d)\n",
> >> We usually recommend to avoid splitting the format string so it is
> >> easier to grep in the code.
> >
> > In this case, the string is longer than 79 characters, so there was 
> > splitting.
>
> Yes, but as I pointed out splitting the string makes more difficult to
> grep for it in the code base. So we prefer to avoid split the string
> even if it is longer than 79 characters.

Ok, clang-format seems doesn't work this way. It needs to investigate
how to implement it.

>
> >
> >>
> >> *
> >>
> >> -# define D11PRINT(fmt, args...) do {} while ( 0 )
> >> +#define D11PRINT(fmt, args...) \
> >> +do {   \
> >> +} while ( 0 )
> >>
> >> It is fairly common to keep everything on a line when the
> >> body is empty. We also use is for stub static inline helper.
> >> I am not sure how difficult it would be to implement that with 
> >> clang-format.
> >
> > Sorry for repeating it again and again, but such cases should be added
> > to the coding style document explicitly.
>
> Patch are always welcome...

Agree with you about it, and this seems to be the hardest thing to overcome.

>
> > It is unknown how difficult it would be to implement that with
> > clang-format, however, it can be analyzed.
>
> ...  but the goal of this discussion is to understand the limitations of
> Clang-format and whether a Coding Style change may be easier.

It is not so easy to do, so it may take a time to investigate every the case.

>
> >>
> >> *
> >>
> >> -/* See linux
> >> Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
> >> +/* See linux
> >> + * Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt 
> >> */
> >>
> >> Multi-lines comment on Xen are using
> >> /*
> >>* Foo
> >>* Bar
> >>*/
> >
> > See my comment about clang-format supports only comments indentation for 
> > now.
>
> I saw it and I will reply here for simplicity. Having a automatic
> checker that will do the wrong things is not ideal.
>
> Imagine we decide to use this checker as a part of the commit process.
> This means that the code will be modified to checker coding style and
> not Xen one.

Well, you are right. Even more, unfortunately, there is no such coding
style as 'bsd' in clang-format.
So 'xen' clang-format style is based on the 'llvm' style.

>
> >
> >>
> >> *
> >>
> >> -const char compat[] =
> >> -"arm,psci-1.0""\0"
> >> -"arm,psci-0.2""\0"
> >> -"arm,psci";
> >> +const char compat[] = "arm,psci-1.0"
> >> +  "\0"
> >> +  "arm,psci-0.2"
> >> +  "\0"
> >> +  "arm,psci";
> >>
> >> I am not sure why clang-format decided to format like that. Do you know 
> >> why?
> >
> > The reason is that there are two strings in one line. It would not
> > change it if it were
> > not "arm,psci-1.0""\0", but "arm,psci-1.0\0".
>
> I would like to see the exact part of the clang-format coding style
> documentation that mention this requirements... The more that in an
> example above (copied below for simplicity), there are two strings in on
> line.

The closest found seems BinPackParameters BinPackArguments,
however, it is about function calls according to manual...

>
>  >> +D11PRINT("Allocated %#" PRIpaddr "-%#" PRIpaddr
>
>
> >
> >>
> >> *
> >>
> >> -clock_valid = dt_property_read_u32(dev, "clock-frequency",
> >> -   _frequency);
> >> +clock_valid =
> >> +dt_property_read_u32(dev, "clock-frequency", _frequency);
> >>
> >> I am not sure why clang-format decide to format like that. The current 
> >> version
> >> is definitely valid.
> >
> > The current version is not valid as it takes 81 chars, while 79 is
> > allowed according to coding style.
>
> Really? I looked at the code and this is 62 characters... It would be 81
> characters if "_frequency);" were on the same line. But see how it
> is split in 2 lines.

You are right, there are two lines. So it needs to find out how to
configure or implement such a feature to ignore such

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-29 Thread Julien Grall 

Hi,

On 7/29/19 10:13 AM, Viktor Mitin wrote:

On Fri, Jul 26, 2019 at 3:50 PM Julien Grall  wrote:


I have already done some testings a couple of weeks ago with the patch [1]. I
have sent some comments regarding the change made by the tools that require some
attention. It would be good if someone go through them and try to address one by
one. For convenience I have replicated my e-mail publicly below.



*** xen/arm/domain_build.c ***

*

-D11PRINT("Allocated %#"PRIpaddr"-%#"PRIpaddr" (%ldMB/%ldMB, order %d)\n",
- start, start + size,
- 1UL << (order + PAGE_SHIFT - 20),
+D11PRINT("Allocated %#" PRIpaddr "-%#" PRIpaddr
+ " (%ldMB/%ldMB, order %d)\n",
We usually recommend to avoid splitting the format string so it is
easier to grep in the code.


In this case, the string is longer than 79 characters, so there was splitting.


Yes, but as I pointed out splitting the string makes more difficult to 
grep for it in the code base. So we prefer to avoid split the string 
even if it is longer than 79 characters.






*

-# define D11PRINT(fmt, args...) do {} while ( 0 )
+#define D11PRINT(fmt, args...) \
+do {   \
+} while ( 0 )

It is fairly common to keep everything on a line when the
body is empty. We also use is for stub static inline helper.
I am not sure how difficult it would be to implement that with clang-format.


Sorry for repeating it again and again, but such cases should be added
to the coding style document explicitly.


Patch are always welcome...


It is unknown how difficult it would be to implement that with
clang-format, however, it can be analyzed.


...  but the goal of this discussion is to understand the limitations of 
Clang-format and whether a Coding Style change may be easier.




*

-/* See linux
Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
+/* See linux
+ * Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */

Multi-lines comment on Xen are using
/*
   * Foo
   * Bar
   */


See my comment about clang-format supports only comments indentation for now. 


I saw it and I will reply here for simplicity. Having a automatic 
checker that will do the wrong things is not ideal.


Imagine we decide to use this checker as a part of the commit process. 
This means that the code will be modified to checker coding style and 
not Xen one.






*

-const char compat[] =
-"arm,psci-1.0""\0"
-"arm,psci-0.2""\0"
-"arm,psci";
+const char compat[] = "arm,psci-1.0"
+  "\0"
+  "arm,psci-0.2"
+  "\0"
+  "arm,psci";

I am not sure why clang-format decided to format like that. Do you know why?


The reason is that there are two strings in one line. It would not
change it if it were
not "arm,psci-1.0""\0", but "arm,psci-1.0\0".


I would like to see the exact part of the clang-format coding style 
documentation that mention this requirements... The more that in an 
example above (copied below for simplicity), there are two strings in on 
line.


>> +D11PRINT("Allocated %#" PRIpaddr "-%#" PRIpaddr






*

-clock_valid = dt_property_read_u32(dev, "clock-frequency",
-   _frequency);
+clock_valid =
+dt_property_read_u32(dev, "clock-frequency", _frequency);

I am not sure why clang-format decide to format like that. The current version
is definitely valid.


The current version is not valid as it takes 81 chars, while 79 is
allowed according to coding style.


Really? I looked at the code and this is 62 characters... It would be 81 
characters if "_frequency);" were on the same line. But see how it 
is split in 2 lines.






*

- got_bank0:
+got_bank0:

IIRC, Jan requests to have a space before the label. Jan?

Jan's answer was:

Yes. No indentation at all for labels leads to them being
(wrongly) used when diff -p tries to identify context. That's
the case even with up-to-date diff iirc; I don't recall
whether git also gets confused by this.


So current clang-format behaviour is correct and nothing to change.


Please read again what was written. Jan confirmed he wanted the space 
before the label. So clear clang-format is not doing the right thing.





*

-const char compat[] =
- "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
-"xen,xen";
+const char compat[] = "xen,xen-" __stringify(XEN_VERSION) "." __stringify(
+XEN_SUBVERSION) "\0"
+"xen,xen";

What is the coding style rule for this change?


It seems the reason for the change is the wrong indentation of the
second line, when the first line has extra space, not sure.


*

-struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
+struct map_range_data mr_data = {.d = d, .p2mt = p2mt};

AFAICT, we commonly put a space after { and before

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-29 Thread Viktor Mitin 
Hi Julien,

On Fri, Jul 26, 2019 at 3:50 PM Julien Grall  wrote:
>
> I have already done some testings a couple of weeks ago with the patch [1]. I
> have sent some comments regarding the change made by the tools that require 
> some
> attention. It would be good if someone go through them and try to address one 
> by
> one. For convenience I have replicated my e-mail publicly below.

> *** xen/arm/domain_build.c ***
>
> *
>
> -D11PRINT("Allocated %#"PRIpaddr"-%#"PRIpaddr" (%ldMB/%ldMB, order %d)\n",
> - start, start + size,
> - 1UL << (order + PAGE_SHIFT - 20),
> +D11PRINT("Allocated %#" PRIpaddr "-%#" PRIpaddr
> + " (%ldMB/%ldMB, order %d)\n",
>We usually recommend to avoid splitting the format string so it is
> easier to grep in the code.

In this case, the string is longer than 79 characters, so there was splitting.

>
> *
>
> -# define D11PRINT(fmt, args...) do {} while ( 0 )
> +#define D11PRINT(fmt, args...) \
> +do {   \
> +} while ( 0 )
>
> It is fairly common to keep everything on a line when the
> body is empty. We also use is for stub static inline helper.
> I am not sure how difficult it would be to implement that with clang-format.

Sorry for repeating it again and again, but such cases should be added
to the coding style document explicitly.
It is unknown how difficult it would be to implement that with
clang-format, however, it can be analyzed.
>
> *
>
> -/* See linux
> Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
> +/* See linux
> + * Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */
>
> Multi-lines comment on Xen are using
> /*
>   * Foo
>   * Bar
>   */

See my comment about clang-format supports only comments indentation for now.

>
> *
>
> -const char compat[] =
> -"arm,psci-1.0""\0"
> -"arm,psci-0.2""\0"
> -"arm,psci";
> +const char compat[] = "arm,psci-1.0"
> +  "\0"
> +  "arm,psci-0.2"
> +  "\0"
> +  "arm,psci";
>
> I am not sure why clang-format decided to format like that. Do you know why?

The reason is that there are two strings in one line. It would not
change it if it were
not "arm,psci-1.0""\0", but "arm,psci-1.0\0".

>
> *
>
> -clock_valid = dt_property_read_u32(dev, "clock-frequency",
> -   _frequency);
> +clock_valid =
> +dt_property_read_u32(dev, "clock-frequency", _frequency);
>
> I am not sure why clang-format decide to format like that. The current version
> is definitely valid.

The current version is not valid as it takes 81 chars, while 79 is
allowed according to coding style.

>
> *
>
> - got_bank0:
> +got_bank0:
>
> IIRC, Jan requests to have a space before the label. Jan?
>
> Jan's answer was:
>
> Yes. No indentation at all for labels leads to them being
> (wrongly) used when diff -p tries to identify context. That's
> the case even with up-to-date diff iirc; I don't recall
> whether git also gets confused by this.
>
So current clang-format behaviour is correct and nothing to change.

> *
>
> -const char compat[] =
> - "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
> -"xen,xen";
> +const char compat[] = "xen,xen-" __stringify(XEN_VERSION) "." 
> __stringify(
> +XEN_SUBVERSION) "\0"
> +"xen,xen";
>
> What is the coding style rule for this change?

It seems the reason for the change is the wrong indentation of the
second line, when the first line has extra space, not sure.

> *
>
> -struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
> +struct map_range_data mr_data = {.d = d, .p2mt = p2mt};
>
> AFAICT, we commonly put a space after { and before }.

This is no explicitly documented in the coding style.
It still seems not an issue to add such cases to clang-format.

> *** xen/arm/mm.c ***
>
>   const unsigned int offsets[4] = {
> -zeroeth_table_offset(addr),
> -first_table_offset(addr),
> -second_table_offset(addr),
> -third_table_offset(addr)
> -};
> +zeroeth_table_offset(addr), first_table_offset(addr),
> +second_table_offset(addr), third_table_offset(addr)};
>
> The old code is technically valid and I find the new code less readable. Why
> clang-format decided to reformat it? I noticed similar things problem with
> prototype.

It is not clear and to be investigated.

>
> *
>
> -rc = guest_physmap_add_entry(current->domain, gaddr_to_gfn(addr),
> - frame, 0, t);
> +rc = guest_physmap_add_entry(current->domain, gaddr_to_gfn(addr), frame, 
> 0,
> + t);
>
> It feels to me that clang-format is trying to cram as much as possible on a
> line.  Can you confirm it?

Seems yes, in this case.

>
> The code per se is valid and

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-26 Thread Lars Kurth 


> On 26 Jul 2019, at 15:17, Viktor Mitin  wrote:
> 
> On Fri, Jul 26, 2019 at 4:52 PM Lars Kurth  wrote:
 thank you for putting this mail together and driving this forward. I added 
 committers@ as well as Doug. I am going to let others respond first.
 I am assuming we are looking for some testing?
>>> 
>>> Yes, you are right.
>>> The implementation has been updated and retested with newer versions
>>> of clang code.
>>> We are looking for some testing and feedback to move forward.
>>> 
 Is there a simple set of instructions to get started and test the tool?
>>> 
>>> Yes, however, since the changes are not integrated into clang-format
>>> mainline yet,
>>> to test the tool it needs to compile clang-format tool with the patch first.
>> 
>> OK
>> Is there a git repo which includes the patch? That would make things a 
>> little easier
> 
> There is no llvm repo with the patch since we checked various releases
> of clang with it
> However, it is a good idea to prepare such a repo to simplify the
> build of the tool.
> We will prepare the repo for it.

Thank you! That makes things easier. I will probably be testing this on a Mac

>>> There are two use-cases with it:
>>> - clang-format binary can be used as-is to check given file or many files.
>>> For example, the next command formats all xen *.c files with it.
>>> find ~/w/xen/xen -name '*.c' -print0 | xargs -0 -n 1 -P 12
>>> ~/w/llvm-project/bin/clang-format -i -style=xen
>>> 
>>> See output example in:
>>> https://raw.githubusercontent.com/viktor-mitin/xen-clang-format-example/master/0001-clang-format-checkpatch-output-example.patch
>>> 
>>> - another use-case is to run it with clang-format diff checker,
>>> For example, the next command line checks the latest commit in case of git:
>>> git diff -U0 --no-color HEAD^ | clang-format-diff -p1
>> 
>> Does this require to copy the modified clang-format-diff.py (which is 
>> mentioned in the mail) somewhere
> 
> Not really, mean it does not require to copy the modified
> clang-format-diff.py. The only feature modified clang-format-diff.py
> provides is covering code file to code style mappings.
> This is a minor feature, and it is not relevant for the Xen code style
> patches testing. It has been decided not to modify python tool for
> now...
> So it is possible to use not modified version of clang-format-diff
> tool for the patches checks.

Cool! 

Regatds
Lars


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

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-26 Thread Viktor Mitin 
On Fri, Jul 26, 2019 at 4:52 PM Lars Kurth  wrote:
> >> thank you for putting this mail together and driving this forward. I added 
> >> committers@ as well as Doug. I am going to let others respond first.
> >> I am assuming we are looking for some testing?
> >
> > Yes, you are right.
> > The implementation has been updated and retested with newer versions
> > of clang code.
> > We are looking for some testing and feedback to move forward.
> >
> >> Is there a simple set of instructions to get started and test the tool?
> >
> > Yes, however, since the changes are not integrated into clang-format
> > mainline yet,
> > to test the tool it needs to compile clang-format tool with the patch first.
>
> OK
> Is there a git repo which includes the patch? That would make things a little 
> easier

There is no llvm repo with the patch since we checked various releases
of clang with it
However, it is a good idea to prepare such a repo to simplify the
build of the tool.
We will prepare the repo for it.

>
> > There are two use-cases with it:
> > - clang-format binary can be used as-is to check given file or many files.
> > For example, the next command formats all xen *.c files with it.
> > find ~/w/xen/xen -name '*.c' -print0 | xargs -0 -n 1 -P 12
> > ~/w/llvm-project/bin/clang-format -i -style=xen
> >
> > See output example in:
> > https://raw.githubusercontent.com/viktor-mitin/xen-clang-format-example/master/0001-clang-format-checkpatch-output-example.patch
> >
> > - another use-case is to run it with clang-format diff checker,
> > For example, the next command line checks the latest commit in case of git:
> > git diff -U0 --no-color HEAD^ | clang-format-diff -p1
>
> Does this require to copy the modified clang-format-diff.py (which is 
> mentioned in the mail) somewhere

Not really, mean it does not require to copy the modified
clang-format-diff.py. The only feature modified clang-format-diff.py
provides is covering code file to code style mappings.
This is a minor feature, and it is not relevant for the Xen code style
patches testing. It has been decided not to modify python tool for
now...
So it is possible to use not modified version of clang-format-diff
tool for the patches checks.

Thanks

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

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-26 Thread Lars Kurth 


> On 26 Jul 2019, at 14:45, Viktor Mitin  wrote:
> 
>> Hi Viktor,
> 
> Hi Lars,
> 
>> thank you for putting this mail together and driving this forward. I added 
>> committers@ as well as Doug. I am going to let others respond first.
>> I am assuming we are looking for some testing?
> 
> Yes, you are right.
> The implementation has been updated and retested with newer versions
> of clang code.
> We are looking for some testing and feedback to move forward.
> 
>> Is there a simple set of instructions to get started and test the tool?
> 
> Yes, however, since the changes are not integrated into clang-format
> mainline yet,
> to test the tool it needs to compile clang-format tool with the patch first.

OK
Is there a git repo which includes the patch? That would make things a little 
easier

> There are two use-cases with it:
> - clang-format binary can be used as-is to check given file or many files.
> For example, the next command formats all xen *.c files with it.
> find ~/w/xen/xen -name '*.c' -print0 | xargs -0 -n 1 -P 12
> ~/w/llvm-project/bin/clang-format -i -style=xen
> 
> See output example in:
> https://raw.githubusercontent.com/viktor-mitin/xen-clang-format-example/master/0001-clang-format-checkpatch-output-example.patch
> 
> - another use-case is to run it with clang-format diff checker,
> For example, the next command line checks the latest commit in case of git:
> git diff -U0 --no-color HEAD^ | clang-format-diff -p1

Does this require to copy the modified clang-format-diff.py (which is mentioned 
in the mail) somewhere

Lars




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

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-26 Thread Viktor Mitin 
Hi Julien,

> I have already done some testings a couple of weeks ago with the patch [1]. I
> have sent some comments regarding the change made by the tools that require 
> some
> attention. It would be good if someone go through them and try to address one 
> by
> one. For convenience I have replicated my e-mail publicly below.

> I would like to also draw the attention to the thread from Tamas about 
> .astylerc
> (https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01145.html).
>

Will go over the thread from Tamas about .astylerc first and will go
over the cases you mentioned after it. See my next emails about it.

Thanks

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

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-26 Thread Viktor Mitin 
> Hi Viktor,

Hi Lars,

> thank you for putting this mail together and driving this forward. I added 
> committers@ as well as Doug. I am going to let others respond first.
> I am assuming we are looking for some testing?

Yes, you are right.
The implementation has been updated and retested with newer versions
of clang code.
We are looking for some testing and feedback to move forward.

> Is there a simple set of instructions to get started and test the tool?

Yes, however, since the changes are not integrated into clang-format
mainline yet,
to test the tool it needs to compile clang-format tool with the patch first.

There are two use-cases with it:
- clang-format binary can be used as-is to check given file or many files.
For example, the next command formats all xen *.c files with it.
find ~/w/xen/xen -name '*.c' -print0 | xargs -0 -n 1 -P 12
~/w/llvm-project/bin/clang-format -i -style=xen

See output example in:
https://raw.githubusercontent.com/viktor-mitin/xen-clang-format-example/master/0001-clang-format-checkpatch-output-example.patch

- another use-case is to run it with clang-format diff checker,
For example, the next command line checks the latest commit in case of git:
 git diff -U0 --no-color HEAD^ | clang-format-diff -p1

Thanks

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

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-26 Thread Julien Grall 



On 26/07/2019 13:42, Lars Kurth wrote:

Hi Viktor,


Hi,

thank you for putting this mail together and driving this forward. I added 
committers@ as well as Doug. I am going to let others respond first.

I am assuming we are looking for some testing?


I have already done some testings a couple of weeks ago with the patch [1]. I 
have sent some comments regarding the change made by the tools that require some 
attention. It would be good if someone go through them and try to address one by 
one. For convenience I have replicated my e-mail publicly below.


I would like to also draw the attention to the thread from Tamas about .astylerc 
(https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg01145.html).


Cheers,

*** xen/arm/domain_build.c ***

*

-D11PRINT("Allocated %#"PRIpaddr"-%#"PRIpaddr" (%ldMB/%ldMB, order %d)\n",
- start, start + size,
- 1UL << (order + PAGE_SHIFT - 20),
+D11PRINT("Allocated %#" PRIpaddr "-%#" PRIpaddr
+ " (%ldMB/%ldMB, order %d)\n",
  We usually recommend to avoid splitting the format string so it is
easier to grep in the code.

*

-# define D11PRINT(fmt, args...) do {} while ( 0 )
+#define D11PRINT(fmt, args...) \
+do {   \
+} while ( 0 )

It is fairly common to keep everything on a line when the
body is empty. We also use is for stub static inline helper.
I am not sure how difficult it would be to implement that with clang-format.

*

-/* See linux 
Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */

+/* See linux
+ * Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt */

Multi-lines comment on Xen are using
/*
 * Foo
 * Bar
 */

*

-const char compat[] =
-"arm,psci-1.0""\0"
-"arm,psci-0.2""\0"
-"arm,psci";
+const char compat[] = "arm,psci-1.0"
+  "\0"
+  "arm,psci-0.2"
+  "\0"
+  "arm,psci";

I am not sure why clang-format decided to format like that. Do you know why?

*

-clock_valid = dt_property_read_u32(dev, "clock-frequency",
-   _frequency);
+clock_valid =
+dt_property_read_u32(dev, "clock-frequency", _frequency);

I am not sure why clang-format decide to format like that. The current version 
is definitely valid.


*

- got_bank0:
+got_bank0:

IIRC, Jan requests to have a space before the label. Jan?

Jan's answer was:

Yes. No indentation at all for labels leads to them being
(wrongly) used when diff -p tries to identify context. That's
the case even with up-to-date diff iirc; I don't recall
whether git also gets confused by this.

*

-const char compat[] =
- "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
-"xen,xen";
+const char compat[] = "xen,xen-" __stringify(XEN_VERSION) "." __stringify(
+XEN_SUBVERSION) "\0"
+"xen,xen";

What is the coding style rule for this change?

*

-struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
+struct map_range_data mr_data = {.d = d, .p2mt = p2mt};

AFAICT, we commonly put a space after { and before }.

*** xen/arm/mm.c ***

 const unsigned int offsets[4] = {
-zeroeth_table_offset(addr),
-first_table_offset(addr),
-second_table_offset(addr),
-third_table_offset(addr)
-};
+zeroeth_table_offset(addr), first_table_offset(addr),
+second_table_offset(addr), third_table_offset(addr)};

The old code is technically valid and I find the new code less readable. Why 
clang-format decided to reformat it? I noticed similar things problem with 
prototype.


*

-rc = guest_physmap_add_entry(current->domain, gaddr_to_gfn(addr),
- frame, 0, t);
+rc = guest_physmap_add_entry(current->domain, gaddr_to_gfn(addr), frame, 0,
+ t);

It feels to me that clang-format is trying to cram as much as possible on a 
line.  Can you confirm it?


The code per se is valid and it feels to me more readable. I would expect 
clang-format to not modify a line if the code is valid per the coding style.


*

-switch ( attr )
+switch (attr)

switch is a logical statement, so we require the space after ( and before ).


--
Julien Grall

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

Re: [Xen-devel] Xen Coding style and clang-format

2019-07-26 Thread Lars Kurth 


> On 26 Jul 2019, at 13:30, Viktor Mitin  wrote:
> 
> Hi All,
> 
> The Xen Project has a coding standard in place, but like many
> projects, the standard is only enforced through peer review. Such
> mistakes slip through and code is imported from other projects which
> may not follow the same standard. The goal would be to come up with a
> tool that can audit the code base as part of a CI loop for code style
> inconsistencies and potentially provide corrections. This tool is to
> embed as a part of the continuous integration loop. For clarity, let’s
> call such a tool as ‘Xen checkpatch tool’.
> 
> References for those who are interested in the background are in [5].
> 
> The idea of the new tool is to use the clang-format approach as a base
> for Xen ‘checkpatch’ process. The new tool consists of modified
> clang-format binary and modified clang-format-diff.py python script to
> automate Xen patches format checking and reformatting. The tool can be
> used as a pre-commit hook to check and format every patch
> automatically. See the tool code under [1].
> 
> Known limitations:
> 
> Xen coding style
> Xen coding style is defined in two 'CODING_STYLE' documents in Xen
> code: Xen common coding style and Xen libxl coding style. The
> documents describe some of the coding style rules. However, there is
> no information about ‘base’ coding style used (i.e. K, Linux, LLVM,
> Google, Chromium, Mozilla, WebKit…). For this reason, it is unclear
> how to deal with some of the coding style rules not described in the
> Xen coding style documents. See examples of the tool output under [2].
> 
> Clang-format
> Generally, the design of clang-format is to only make formatting
> changes, not adding or removing tokens (there are some exceptions to
> this, like wrapping string literals). It means that clang-format can't
> add or remove braces or change the style of the comments from C89 to
> C++. Tool clang-tidy can make syntactic changes to the code. However,
> unfortunately, clang-tidy is a heavyweight tool as it needs the
> compile options to parse the file (See [3] and [4])
> 
> This can be clang generic limitation, e.g. we might want to add a
> possibility to clang to alter the code, e.g. adding braces,
> characters, etc". The concern here is that it seems it is against main
> clang-format design principles, so those changes will not be
> integrated into clang-format mainstream. It should be checked with
> clang-format community first.
> 
> As an option, to overcome the limitations of clang-format tool in the
> case of Xen coding style, it is possible to move some Xen code format
> logic to the modified clang-format-diff.py tool.
> 
> Summary
> To sum up, it is possible to automate Xen patches format checking and
> corrections with some known clang-format limitations. Ideally, it
> would be good to slowly migrate the entire code-base to be conforming,
> thus eliminating the need for discussing and enforcing style issues
> manually on the mailing list. The ‘Xen checkpatch tool’ provides the
> closest approximation of the established Xen style (including styles
> not formally spelled out by CODING_STYLE, but commonly requested).
> The tool can be used as-is at the moment and improved later in case of
> necessity.
> 
> The tool allows achieving automation of Xen patches format checking
> and corrections with some known clang-format limitations (see below).
> All the xen/*.c files have been tested with it.
> See the results of the tool output under [2].
> 
> Summary of the changes:
> - Added 3 new formatting styles to cover all the cases mentioned in
> Xen coding style document: Xen, Libxl, Linux;
> - Added list of the files and corresponding style name mappings;
> - Added indentation according to Xen coding style;
> - Added white space formatting according to Xen coding style;
> - Added bracing support exception for do/while loops;
> 
> Added to clang-format, however, probably this logic should be moved to
> python part (see known clang-format limitations above):
> - Braces should be omitted for blocks with a single statement. Note:
> these braces will be required by MISRA, for example, so it is probably
> worth adding such a requirement to the coding style.
> - Comments format requirements. Note: //-style comments are defined in
> C99 as well, and not just in the case of C++. C99 standard is 20-years
> old…
> 
> To be added:
> - Emacs local variables. Open points: Why to keep emacs local
> variables in Xen code? What about other editors' comments (vim)?
> - Warning to stderr in the case when ‘unfixable’ line/s detected.
> 
> To be fixed:
> - Max line length from 80 to 79 chars;
> - Disable // comments;
> 
> 
> The links:
> [1] 
> https://github.com/xen-troops/Xen-Clang-format/blob/devel/clang-format.patch
> [2] 
> https://raw.githubusercontent.com/viktor-mitin/xen-clang-format-example/master/0001-clang-format-checkpatch-output-example.patch
> [3] https://developer.blender.org/T53211
> [4] 
>

[Xen-devel] Xen Coding style and clang-format

2019-07-26 Thread Viktor Mitin 
Hi All,

The Xen Project has a coding standard in place, but like many
projects, the standard is only enforced through peer review. Such
mistakes slip through and code is imported from other projects which
may not follow the same standard. The goal would be to come up with a
tool that can audit the code base as part of a CI loop for code style
inconsistencies and potentially provide corrections. This tool is to
embed as a part of the continuous integration loop. For clarity, let’s
call such a tool as ‘Xen checkpatch tool’.

References for those who are interested in the background are in [5].

The idea of the new tool is to use the clang-format approach as a base
for Xen ‘checkpatch’ process. The new tool consists of modified
clang-format binary and modified clang-format-diff.py python script to
automate Xen patches format checking and reformatting. The tool can be
used as a pre-commit hook to check and format every patch
automatically. See the tool code under [1].

Known limitations:

Xen coding style
Xen coding style is defined in two 'CODING_STYLE' documents in Xen
code: Xen common coding style and Xen libxl coding style. The
documents describe some of the coding style rules. However, there is
no information about ‘base’ coding style used (i.e. K, Linux, LLVM,
Google, Chromium, Mozilla, WebKit…). For this reason, it is unclear
how to deal with some of the coding style rules not described in the
Xen coding style documents. See examples of the tool output under [2].

Clang-format
Generally, the design of clang-format is to only make formatting
changes, not adding or removing tokens (there are some exceptions to
this, like wrapping string literals). It means that clang-format can't
add or remove braces or change the style of the comments from C89 to
C++. Tool clang-tidy can make syntactic changes to the code. However,
unfortunately, clang-tidy is a heavyweight tool as it needs the
compile options to parse the file (See [3] and [4])

This can be clang generic limitation, e.g. we might want to add a
possibility to clang to alter the code, e.g. adding braces,
characters, etc". The concern here is that it seems it is against main
clang-format design principles, so those changes will not be
integrated into clang-format mainstream. It should be checked with
clang-format community first.

As an option, to overcome the limitations of clang-format tool in the
case of Xen coding style, it is possible to move some Xen code format
logic to the modified clang-format-diff.py tool.

Summary
To sum up, it is possible to automate Xen patches format checking and
corrections with some known clang-format limitations. Ideally, it
would be good to slowly migrate the entire code-base to be conforming,
thus eliminating the need for discussing and enforcing style issues
manually on the mailing list. The ‘Xen checkpatch tool’ provides the
closest approximation of the established Xen style (including styles
not formally spelled out by CODING_STYLE, but commonly requested).
The tool can be used as-is at the moment and improved later in case of
necessity.

The tool allows achieving automation of Xen patches format checking
and corrections with some known clang-format limitations (see below).
All the xen/*.c files have been tested with it.
See the results of the tool output under [2].

Summary of the changes:
- Added 3 new formatting styles to cover all the cases mentioned in
Xen coding style document: Xen, Libxl, Linux;
- Added list of the files and corresponding style name mappings;
- Added indentation according to Xen coding style;
- Added white space formatting according to Xen coding style;
- Added bracing support exception for do/while loops;

Added to clang-format, however, probably this logic should be moved to
python part (see known clang-format limitations above):
- Braces should be omitted for blocks with a single statement. Note:
these braces will be required by MISRA, for example, so it is probably
worth adding such a requirement to the coding style.
- Comments format requirements. Note: //-style comments are defined in
C99 as well, and not just in the case of C++. C99 standard is 20-years
old…

To be added:
- Emacs local variables. Open points: Why to keep emacs local
variables in Xen code? What about other editors' comments (vim)?
- Warning to stderr in the case when ‘unfixable’ line/s detected.

To be fixed:
- Max line length from 80 to 79 chars;
- Disable // comments;


The links:
[1] https://github.com/xen-troops/Xen-Clang-format/blob/devel/clang-format.patch
[2] 
https://raw.githubusercontent.com/viktor-mitin/xen-clang-format-example/master/0001-clang-format-checkpatch-output-example.patch
[3] https://developer.blender.org/T53211
[4] 
http://clang-developers.42468.n3.nabble.com/clang-format-add-around-statement-after-if-while-for-td4049620.html

[5]
Project status: