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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel