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

[Viktor Mitin <viktor.mitin.19@xxxxxxxxx>]

Hi Julien,

On Fri, Jul 26, 2019 at 3:50 PM Julien Grall <julien.grall@xxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel