Hi,
On 7/29/19 10:13 AM, Viktor Mitin wrote:
On Fri, Jul 26, 2019 at 3:50 PM Julien Grall <julien.gr...@arm.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
