|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] Xen Coding style and clang-format
Hi Julien,
On Mon, Jul 29, 2019 at 1:49 PM Julien Grall <julien.grall@xxxxxxx> 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@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.
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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |