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

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

[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&R, 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:
https://docs.google.com/document/d/10NJn-QvO1TvyJJJGE2PD6FtElYCT3neBAffIqeWHdiE/edithttps://docs.google.com/document/d/1tCcwZ9K38ToLGTPHZkfs2sS4s4YulrGoH8LIHwBMbg4/edit

Mailing list discussions:
https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg02848.html
https://lists.xenproject.org/archives/html/xen-devel/2017-04/msg00131.html
https://lists.xenproject.org/archives/html/xen-devel/2017-04/msg01739.html

Original implementation on GitHub:
https://github.com/sam5125/Xen-Clang-format


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

[Lars Kurth]

[-- Attachment #1.1: Type: text/plain, Size: 6405 bytes --]



> On 26 Jul 2019, at 13:30, Viktor Mitin <viktor.mitin.19@gmail.com> 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&R, 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:
> https://docs.google.com/document/d/10NJn-QvO1TvyJJJGE2PD6FtElYCT3neBAffIqeWHdiE/edit <https://docs.google.com/document/d/10NJn-QvO1TvyJJJGE2PD6FtElYCT3neBAffIqeWHdiE/edit>
This seems to be an old outreachy document

> https://docs.google.com/document/d/1tCcwZ9K38ToLGTPHZkfs2sS4s4YulrGoH8LIHwBMbg4/edit <https://docs.google.com/document/d/1tCcwZ9K38ToLGTPHZkfs2sS4s4YulrGoH8LIHwBMbg4/edit>

I suppose this is the project status?

> Mailing list discussions:
> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg02848.html
> https://lists.xenproject.org/archives/html/xen-devel/2017-04/msg00131.html
> https://lists.xenproject.org/archives/html/xen-devel/2017-04/msg01739.html
> 
> Original implementation on GitHub:
> https://github.com/sam5125/Xen-Clang-format <https://github.com/sam5125/Xen-Clang-format>

Hi Viktor,

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?

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

Regards
Lars



[-- Attachment #1.2: Type: text/html, Size: 8877 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
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

[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",
-                                       &clock_frequency);
+    clock_valid =
+        dt_property_read_u32(dev, "clock-frequency", &clock_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

[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

[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

[Lars Kurth]

> On 26 Jul 2019, at 14:45, Viktor Mitin <viktor.mitin.19@gmail.com> 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

[Viktor Mitin]

On Fri, Jul 26, 2019 at 4:52 PM Lars Kurth <lars.kurth.xen@gmail.com> 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

[Lars Kurth]

> On 26 Jul 2019, at 15:17, Viktor Mitin <viktor.mitin.19@gmail.com> wrote:
> 
> On Fri, Jul 26, 2019 at 4:52 PM Lars Kurth <lars.kurth.xen@gmail.com> 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

[Viktor Mitin]

Hi Julien,

On Fri, Jul 26, 2019 at 3:50 PM Julien Grall <julien.grall@arm.com> 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",
> -                                       &clock_frequency);
> +    clock_valid =
> +        dt_property_read_u32(dev, "clock-frequency", &clock_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 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.

The thing is what is the definition of "more readable" and "valid per
the coding style".
In this case, it tries to use all of the 79 characters of the line.

> *****
>
> -    switch ( attr )
> +    switch (attr)
>
> switch is a logical statement, so we require the space after ( and before ).

This is to be added to implementation.

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

[Julien Grall]

Hi,

On 7/29/19 10:13 AM, Viktor Mitin wrote:
> On Fri, Jul 26, 2019 at 3:50 PM Julien Grall <julien.grall@arm.com> 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",
>> -                                       &clock_frequency);
>> +    clock_valid =
>> +        dt_property_read_u32(dev, "clock-frequency", &clock_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 "&clock_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 }.
> 
> 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 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.
> 
> The thing is what is the definition of "more readable" and "valid per
> the coding style".
> In this case, it tries to use all of the 79 characters of the line.

What's the rationale here? Do you have the exact section in the 
clang-format coding style for this?

This is one case where I feel the checker is imposing a lot more 
restriction than it should do.

There are a lot of cases where cramming everything in one line is 
possible. But sometimes, you may want to do it over multiple lines for 
more readability (this is pretty subjective). As a reviewer, I would 
accept both cases. But I would clearly not impose on the contributor 
either way.

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

[Viktor Mitin]

Hi Julien,

On Mon, Jul 29, 2019 at 1:49 PM Julien Grall <julien.grall@arm.com> wrote:
>
> Hi,
>
> On 7/29/19 10:13 AM, Viktor Mitin wrote:
> > On Fri, Jul 26, 2019 at 3:50 PM Julien Grall <julien.grall@arm.com> 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",
> >> -                                       &clock_frequency);
> >> +    clock_valid =
> >> +        dt_property_read_u32(dev, "clock-frequency", &clock_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 "&clock_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.
>
> >
> >>
> >> *****
> >>
> >> - 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.

Ok, if we agree such a rule in coding style document, then let's implement such.
>
> >
> >> *****
> >>
> >> -    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 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.
> >
> > The thing is what is the definition of "more readable" and "valid per
> > the coding style".
> > In this case, it tries to use all of the 79 characters of the line.
>
> What's the rationale here? Do you have the exact section in the
> clang-format coding style for this?
>
> This is one case where I feel the checker is imposing a lot more
> restriction than it should do.
>
> There are a lot of cases where cramming everything in one line is
> possible. But sometimes, you may want to do it over multiple lines for
> more readability (this is pretty subjective). As a reviewer, I would
> accept both cases. But I would clearly not impose on the contributor
> either way.

Agree, the best option is to find out how to ignore such cases with
clang-format.

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

[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 <julien.grall@arm.com> wrote:
>> On 7/29/19 10:13 AM, Viktor Mitin wrote:
>>> On Fri, Jul 26, 2019 at 3:50 PM Julien Grall <julien.grall@arm.com> 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",
>>>> -                                       &clock_frequency);
>>>> +    clock_valid =
>>>> +        dt_property_read_u32(dev, "clock-frequency", &clock_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 "&clock_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

[Viktor Mitin]

Hi Julien,

On Mon, Jul 29, 2019 at 3:35 PM Julien Grall <julien.grall@arm.com> wrote:
>
> Hi Viktor,
>
> On 7/29/19 1:21 PM, Viktor Mitin wrote:
> > On Mon, Jul 29, 2019 at 1:49 PM Julien Grall <julien.grall@arm.com> wrote:
> >> On 7/29/19 10:13 AM, Viktor Mitin wrote:
> >>> On Fri, Jul 26, 2019 at 3:50 PM Julien Grall <julien.grall@arm.com> 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",
> >>>> -                                       &clock_frequency);
> >>>> +    clock_valid =
> >>>> +        dt_property_read_u32(dev, "clock-frequency", &clock_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 "&clock_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

[Julien Grall]

On 31/07/2019 12:16, Viktor Mitin wrote:
> On Mon, Jul 29, 2019 at 3:35 PM Julien Grall <julien.grall@arm.com> wrote:
>> On 7/29/19 1:21 PM, Viktor Mitin wrote:
>>> On Mon, Jul 29, 2019 at 1:49 PM Julien Grall <julien.grall@arm.com> wrote:
>>>> On 7/29/19 10:13 AM, Viktor Mitin wrote:
>>>>> On Fri, Jul 26, 2019 at 3:50 PM Julien Grall <julien.grall@arm.com> 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",
>>>>>> -                                       &clock_frequency);
>>>>>> +    clock_valid =
>>>>>> +        dt_property_read_u32(dev, "clock-frequency", &clock_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 "&clock_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

[Viktor Mitin]

On Wed, Jul 31, 2019 at 2:25 PM Julien Grall <julien.grall@arm.com> wrote:
>
>
>
> On 31/07/2019 12:16, Viktor Mitin wrote:
> > On Mon, Jul 29, 2019 at 3:35 PM Julien Grall <julien.grall@arm.com> wrote:
> >> On 7/29/19 1:21 PM, Viktor Mitin wrote:
> >>> On Mon, Jul 29, 2019 at 1:49 PM Julien Grall <julien.grall@arm.com> wrote:
> >>>> On 7/29/19 10:13 AM, Viktor Mitin wrote:
> >>>>> On Fri, Jul 26, 2019 at 3:50 PM Julien Grall <julien.grall@arm.com> 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 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?

See the link above. Unfortunately, it is about C++ and not about C.
Seems there is no pure C support in clang-format.

>
> >
> >>
> >>>
> >>>>
> >>>>    >> +    D11PRINT("Allocated %#" PRIpaddr "-%#" PRIpaddr
> >>>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>> *****
> >>>>>>
> >>>>>> -    clock_valid = dt_property_read_u32(dev, "clock-frequency",
> >>>>>> -                                       &clock_frequency);
> >>>>>> +    clock_valid =
> >>>>>> +        dt_property_read_u32(dev, "clock-frequency", &clock_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 "&clock_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.

See the link above, hopefully it answers your question.

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

[Lars Kurth]

[-- Attachment #1.1: Type: text/plain, Size: 8496 bytes --]



> On 31 Jul 2019, at 12:43, Viktor Mitin <viktor.mitin.19@gmail.com> wrote:
> 
> On Wed, Jul 31, 2019 at 2:25 PM Julien Grall <julien.grall@arm.com <mailto:julien.grall@arm.com>> wrote:
>> 
>> 
>> 
>> On 31/07/2019 12:16, Viktor Mitin wrote:
>>> On Mon, Jul 29, 2019 at 3:35 PM Julien Grall <julien.grall@arm.com> wrote:
>>>> On 7/29/19 1:21 PM, Viktor Mitin wrote:
>>>>> On Mon, Jul 29, 2019 at 1:49 PM Julien Grall <julien.grall@arm.com> wrote:
>>>>>> On 7/29/19 10:13 AM, Viktor Mitin wrote:
>>>>>>> On Fri, Jul 26, 2019 at 3:50 PM Julien Grall <julien.grall@arm.com> 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 <https://clang.llvm.org/docs/ClangFormatStyleOptions.html>
> 
> And LLVM Coding Standards documetation:
> https://llvm.org/docs/CodingStandards.html#introduction <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 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?
> 
> See the link above. Unfortunately, it is about C++ and not about C.
> Seems there is no pure C support in clang-format.
> 
>> 
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>>>> +    D11PRINT("Allocated %#" PRIpaddr "-%#" PRIpaddr
>>>>>> 
>>>>>> 
>>>>>>> 
>>>>>>>> 
>>>>>>>> *****
>>>>>>>> 
>>>>>>>> -    clock_valid = dt_property_read_u32(dev, "clock-frequency",
>>>>>>>> -                                       &clock_frequency);
>>>>>>>> +    clock_valid =
>>>>>>>> +        dt_property_read_u32(dev, "clock-frequency", &clock_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 "&clock_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.
> 
> See the link above, hopefully it answers your question.
> 
> Thanks

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

Regards
Lars


[-- Attachment #1.2: Type: text/html, Size: 45936 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
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

[Viktor Mitin]

On Wed, Jul 31, 2019 at 7:27 PM Lars Kurth <lars.kurth.xen@gmail.com> 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

[Viktor Mitin]

On Wed, Jul 31, 2019 at 8:05 PM Viktor Mitin <viktor.mitin.19@gmail.com> wrote:
>
> On Wed, Jul 31, 2019 at 7:27 PM Lars Kurth <lars.kurth.xen@gmail.com> 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

[Tamas K Lengyel]

On Thu, Aug 1, 2019 at 4:06 AM Viktor Mitin <viktor.mitin.19@gmail.com> wrote:
>
> On Wed, Jul 31, 2019 at 8:05 PM Viktor Mitin <viktor.mitin.19@gmail.com> wrote:
> >
> > On Wed, Jul 31, 2019 at 7:27 PM Lars Kurth <lars.kurth.xen@gmail.com> 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

[Viktor Mitin]

Hi Tamas,

On Thu, Aug 1, 2019 at 6:58 PM Tamas K Lengyel
<tamas.k.lengyel@gmail.com> wrote:
>
> On Thu, Aug 1, 2019 at 4:06 AM Viktor Mitin <viktor.mitin.19@gmail.com> wrote:
> >
> > On Wed, Jul 31, 2019 at 8:05 PM Viktor Mitin <viktor.mitin.19@gmail.com> wrote:
> > >
> > > On Wed, Jul 31, 2019 at 7:27 PM Lars Kurth <lars.kurth.xen@gmail.com> 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