[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





On 31 Jul 2019, at 12:43, Viktor Mitin <viktor.mitin.19@xxxxxxxxx> wrote:

On Wed, Jul 31, 2019 at 2:25 PM Julien Grall <julien.grall@xxxxxxx> wrote:



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

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

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.