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

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

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