| Message ID | 20250216102108.2665222-1-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
On 16/02/2025 10:21 am, Oleksandr Andrushchenko wrote: > There are two diff files which show what happens in case the same is > applied to the whole xen/drivers directory: > - first one is the result of the "git diff" command, 1.2M [3] > - the second one is for "git diff --ignire-all-space", 600K [4] Please can you format everything, and put it on a branch somewhere, so people can browse. ~Andrew
Hello, Roger! Please find the branch with all the conversions [1]. Unfortunately I cannot provide a branch as seen with diff --ignore-all-space as such a patch will not simply apply. Stay safe, Oleksandr Andrushchenko On 16.02.25 13:58, Andrew Cooper wrote: > On 16/02/2025 10:21 am, Oleksandr Andrushchenko wrote: >> There are two diff files which show what happens in case the same is >> applied to the whole xen/drivers directory: >> - first one is the result of the "git diff" command, 1.2M [3] >> - the second one is for "git diff --ignire-all-space", 600K [4] > Please can you format everything, and put it on a branch somewhere, so > people can browse. > > ~Andrew [1] https://github.com/andr2000/xen/tree/clang_ml_drivers_v002_diff
On 16.02.2025 11:21, Oleksandr Andrushchenko wrote: > 1. Const string arrays reformatting > In case the length of items change we might need to introduce a bigger > change wrt new formatting of unaffected lines > ============================================================================== > > --- a/xen/drivers/acpi/tables.c > +++ b/xen/drivers/acpi/tables.c > @@ -38,10 +38,10 @@ > -static const char *__initdata > -mps_inti_flags_polarity[] = { "dfl", "high", "res", "low" }; > -static const char *__initdata > -mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" }; > +static const char *__initdata mps_inti_flags_polarity[] = { "dfl", "high", > + "res", "low" }; > +static const char *__initdata mps_inti_flags_trigger[] = { "dfl", "edge", "res", > > --- a/xen/drivers/acpi/utilities/utglobal.c > +++ b/xen/drivers/acpi/utilities/utglobal.c > static const char *const acpi_gbl_region_types[ACPI_NUM_PREDEFINED_REGIONS] = { > - "SystemMemory", > - "SystemIO", > - "PCI_Config", > - "EmbeddedControl", > - "SMBus", > - "CMOS", > - "PCIBARTarget", > - "DataTable" > + "SystemMemory", "SystemIO", "PCI_Config", "EmbeddedControl", > + "SMBus", "CMOS", "PCIBARTarget", "DataTable" > }; Why in the world would a tool need to touch anything like the two examples above? My take is that the code is worse readability-wise afterwards. > 2. Long strings in ptintk violations > I filed an issue on printk format strings [5] > ============================================================================== > @@ -225,9 +231,9 @@ void __init acpi_table_print_madt_entry(struct acpi_subtable_header *header) > printk(KERN_DEBUG PREFIX > - "GIC Distributor (gic_id[0x%04x] address[0x%"PRIx64"] gsi_base[%d])\n", > - p->gic_id, p->base_address, > - p->global_irq_base); > + "GIC Distributor (gic_id[0x%04x] address[0x%" PRIx64 > + "] gsi_base[%d])\n", > + p->gic_id, p->base_address, p->global_irq_base); > > @@ -629,12 +628,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header) > - printk(XENLOG_ERR VTDPREFIX > - "Overlapping RMRRs [%"PRIx64",%"PRIx64"] and [%"PRIx64",%"PRIx64"]\n", > - rmrru->base_address, rmrru->end_address, > - base_addr, end_addr); > + printk(XENLOG_ERR VTDPREFIX "Overlapping RMRRs [%" PRIx64 > + ",%" PRIx64 "] and [%" PRIx64 > + ",%" PRIx64 "]\n", > + rmrru->base_address, rmrru->end_address, base_addr, > + end_addr); This yet more definitely needs avoiding. Seeing that e.g. Linux also makes line length exceptions for format string literals, I would seem pretty likely that there already is a control to leave such alone. > 3. String concatenation after clang - needs manual work to fix > ============================================================================== > --- a/xen/drivers/acpi/apei/apei-base.c > +++ b/xen/drivers/acpi/apei/apei-base.c > @@ -171,16 +169,19 @@ int __apei_exec_run(struct apei_exec_context *ctx, u8 action, > printk(KERN_WARNING > - "Invalid action table, unknown instruction " > - "type: %d\n", entry->instruction); > + "Invalid action table, unknown instruction " "type: %d\n", > + entry->instruction); Urgh. Why would it not join together the two literals? > 4. Looks a bit weird, but correct > ============================================================================== > --- a/xen/drivers/acpi/apei/apei-io.c > +++ b/xen/drivers/acpi/apei/apei-io.c > @@ -80,13 +78,15 @@ static void __iomem *__init apei_range_map(paddr_t paddr, unsigned long size) > - pg = ((((paddr + size -1) & PAGE_MASK) > - - (paddr & PAGE_MASK)) >> PAGE_SHIFT) + 1; > + pg = ((((paddr + size - 1) & PAGE_MASK) - (paddr & PAGE_MASK)) >> > + PAGE_SHIFT) + > + 1; It trying to squash as much on the 1st line as it can, does it really put the lone "1;" at a separate line? > 7. Parentheses for empty functions > ============================================================================== > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -1311,7 +1307,9 @@ void panic(const char *fmt, ...) > -static void cf_check suspend_steal_fn(const char *str, size_t nr) { } > +static void cf_check suspend_steal_fn(const char *str, size_t nr) > +{} While not the end of the world, an option for keeping the braces on the same line surely would be worthwhile for them to support in the tool? > 8. Const struct reformatting is weird and inconsistent > ============================================================================== > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -1050,135 +1055,93 @@ static const struct ns16550_config __initconst uart_config[] = > .param = param_oxford, > }, > /* Pericom PI7C9X7951 Uno UART */ > - { > - .vendor_id = PCI_VENDOR_ID_PERICOM, > + { .vendor_id = PCI_VENDOR_ID_PERICOM, > .dev_id = 0x7951, > - .param = param_pericom_1port > - }, > + .param = param_pericom_1port }, A matter of some control that needs flipping? Readability again suffers here quite a bit, imo. > 9. Define is longer than 80 chars > ============================================================================== > --- a/xen/drivers/cpufreq/cpufreq_ondemand.c > +++ b/xen/drivers/cpufreq/cpufreq_ondemand.c > @@ -27,10 +27,8 @@ > -#define MIN_STAT_SAMPLING_RATE \ > - (MIN_SAMPLING_MILLISECS * MILLISECS(1)) > -#define MIN_SAMPLING_RATE \ > - (def_sampling_rate / MIN_SAMPLING_RATE_RATIO) > +#define MIN_STAT_SAMPLING_RATE (MIN_SAMPLING_MILLISECS * MILLISECS(1)) > +#define MIN_SAMPLING_RATE (def_sampling_rate / MIN_SAMPLING_RATE_RATIO) Oops. Transformed code violating a fundamental rule? > 10. Union memebers require an empty line between them > ============================================================================== > --- a/xen/drivers/passthrough/amd/iommu-defs.h > +++ b/xen/drivers/passthrough/amd/iommu-defs.h > @@ -289,6 +289,7 @@ struct amd_iommu_dte { > > union amd_iommu_control { > uint64_t raw; > + > struct { This might be acceptable. It's a little wasteful for small unions, but may be quite helpful for larger ones. > 11. Multiline string alignment for at the first string, not for the function > opening bracket. Depends on the macro before the string? > ============================================================================== > --- a/xen/drivers/passthrough/pci.c > +++ b/xen/drivers/passthrough/pci.c > @@ -748,18 +757,18 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, > printk(XENLOG_WARNING "SR-IOV device %pp has its virtual" > - " functions already enabled (%04x)\n", &pdev->sbdf, ctrl); > + " functions already enabled (%04x)\n", > + &pdev->sbdf, ctrl); This kind of line wrapping wants manual adjustment up front then, imo: printk(XENLOG_WARNING "SR-IOV device %pp has its virtual functions already enabled (%04x)\n", &pdev->sbdf, ctrl); Unless of course the tool can be convinced to do the transformations this way in the first place. > 11. const struct initializers are inconsistent > I have filed a bug on clang-format for that [7] > ============================================================================== > > [snip] > static const struct ns16550_config __initconst uart_config[] = { > [snip] > /* OXPCIe200 1 Native UART */ > { > .vendor_id = PCI_VENDOR_ID_OXSEMI, > .dev_id = 0xc4cf, > .param = param_oxford, > }, > /* Pericom PI7C9X7951 Uno UART */ > { .vendor_id = PCI_VENDOR_ID_PERICOM, > .dev_id = 0x7951, > .param = param_pericom_1port }, > [snip] Odd; related to point 8 I think. Jan
On Mon, 17 Feb 2025, Jan Beulich wrote: > On 16.02.2025 11:21, Oleksandr Andrushchenko wrote: > > 1. Const string arrays reformatting > > In case the length of items change we might need to introduce a bigger > > change wrt new formatting of unaffected lines > > ============================================================================== > > > > --- a/xen/drivers/acpi/tables.c > > +++ b/xen/drivers/acpi/tables.c > > @@ -38,10 +38,10 @@ > > -static const char *__initdata > > -mps_inti_flags_polarity[] = { "dfl", "high", "res", "low" }; > > -static const char *__initdata > > -mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" }; > > +static const char *__initdata mps_inti_flags_polarity[] = { "dfl", "high", > > + "res", "low" }; > > +static const char *__initdata mps_inti_flags_trigger[] = { "dfl", "edge", "res", > > > > --- a/xen/drivers/acpi/utilities/utglobal.c > > +++ b/xen/drivers/acpi/utilities/utglobal.c > > static const char *const acpi_gbl_region_types[ACPI_NUM_PREDEFINED_REGIONS] = { > > - "SystemMemory", > > - "SystemIO", > > - "PCI_Config", > > - "EmbeddedControl", > > - "SMBus", > > - "CMOS", > > - "PCIBARTarget", > > - "DataTable" > > + "SystemMemory", "SystemIO", "PCI_Config", "EmbeddedControl", > > + "SMBus", "CMOS", "PCIBARTarget", "DataTable" > > }; > > Why in the world would a tool need to touch anything like the two examples > above? My take is that the code is worse readability-wise afterwards. I think the output is acceptable: not necessarily better than before, but also not significantly worse. To me, the main takeaway is that there are many unavoidable but unnecessary changes.
On 16.02.25 12:21, Oleksandr Andrushchenko wrote: > Hello, everybody! > > As agreed before [1] I am sending a series to show two samples of the > formatting done with clang-format. The clang-format configuration can be > found at [2]. It already has some notes on previous discussions when > Luca presented his version of that configuration and which I used to > start from. > > There are two diff files which show what happens in case the same is > applied to the whole xen/drivers directory: > - first one is the result of the "git diff" command, 1.2M [3] > - the second one is for "git diff --ignire-all-space", 600K [4] > > This is not to limit the reviewers from seeing a bigger picture and not > only the files in this series. > N.B. xen/drivers uses tabs a lot, so this is no surprise the size > difference is big enough: roughly space conversion is half of the changes. > > While reviewing the changes I have collected some of the unexpected things > done by clang-format and some interesting pieces. You can find those > below. For some of those I have filed an issue on clang-format and hope the > community will lead me in resolving those. Of course what I collected is > not the complete list of such changes, so I hope we can discuss missed > ones here. > > From this exercise I can definitely tell that clang-format does help a > lot and has potential to be employed as a code formatting tool for Xen. > Of course it cannot be used as is now and will require discussions on the > Xen coding style and possibly submitting patches to clang-format to > satisfy those which cannot be handled by the tool now. > > Stay safe, > Oleksandr Andrushchenko > > 1. Const string arrays reformatting > In case the length of items change we might need to introduce a bigger > change wrt new formatting of unaffected lines > ============================================================================== > > --- a/xen/drivers/acpi/tables.c > +++ b/xen/drivers/acpi/tables.c > @@ -38,10 +38,10 @@ > -static const char *__initdata > -mps_inti_flags_polarity[] = { "dfl", "high", "res", "low" }; > -static const char *__initdata > -mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" }; > +static const char *__initdata mps_inti_flags_polarity[] = { "dfl", "high", > + "res", "low" }; > +static const char *__initdata mps_inti_flags_trigger[] = { "dfl", "edge", "res", > > --- a/xen/drivers/acpi/utilities/utglobal.c > +++ b/xen/drivers/acpi/utilities/utglobal.c > static const char *const acpi_gbl_region_types[ACPI_NUM_PREDEFINED_REGIONS] = { > - "SystemMemory", > - "SystemIO", > - "PCI_Config", > - "EmbeddedControl", > - "SMBus", > - "CMOS", > - "PCIBARTarget", > - "DataTable" > + "SystemMemory", "SystemIO", "PCI_Config", "EmbeddedControl", > + "SMBus", "CMOS", "PCIBARTarget", "DataTable" > }; > > 2. Long strings in ptintk violations > I filed an issue on printk format strings [5] > ============================================================================== > @@ -225,9 +231,9 @@ void __init acpi_table_print_madt_entry(struct acpi_subtable_header *header) > printk(KERN_DEBUG PREFIX > - "GIC Distributor (gic_id[0x%04x] address[0x%"PRIx64"] gsi_base[%d])\n", > - p->gic_id, p->base_address, > - p->global_irq_base); > + "GIC Distributor (gic_id[0x%04x] address[0x%" PRIx64 > + "] gsi_base[%d])\n", > + p->gic_id, p->base_address, p->global_irq_base); > > @@ -629,12 +628,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header) > - printk(XENLOG_ERR VTDPREFIX > - "Overlapping RMRRs [%"PRIx64",%"PRIx64"] and [%"PRIx64",%"PRIx64"]\n", > - rmrru->base_address, rmrru->end_address, > - base_addr, end_addr); > + printk(XENLOG_ERR VTDPREFIX "Overlapping RMRRs [%" PRIx64 > + ",%" PRIx64 "] and [%" PRIx64 > + ",%" PRIx64 "]\n", > + rmrru->base_address, rmrru->end_address, base_addr, > + end_addr); It doesn't understand properly things like "PRIx64" :( Most of other items, I've mentioned already, > Unhappy: it's probably "known" things - identification of complex defines and > static struct/arrays declarations. And for them, probably, no magic automation tools exist which will satisfy everybody as, at least static definitions are usually formatted to achieve better readability which is always subjective. The tags /* clang-format off/clang-format on */ might be useful. [..] Honestly, It looks a bit strange that Xen community is considering batch automated code formatting, For example Linux kernel cleanly rejected such approach. Linux kernel docs "4.1.1. Coding style" section [1]. Another thing, checking the new code and accepting code style violations (if any) on per-patch basis. [1] https://docs.kernel.org/process/4.Coding.html BR, -grygorii
Hi Oleksandr, > > 2. Long strings in ptintk violations > I filed an issue on printk format strings [5] > ============================================================================== > @@ -225,9 +231,9 @@ void __init acpi_table_print_madt_entry(struct acpi_subtable_header *header) > printk(KERN_DEBUG PREFIX > - "GIC Distributor (gic_id[0x%04x] address[0x%"PRIx64"] gsi_base[%d])\n", > - p->gic_id, p->base_address, > - p->global_irq_base); > + "GIC Distributor (gic_id[0x%04x] address[0x%" PRIx64 > + "] gsi_base[%d])\n", > + p->gic_id, p->base_address, p->global_irq_base); > > @@ -629,12 +628,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header) > - printk(XENLOG_ERR VTDPREFIX > - "Overlapping RMRRs [%"PRIx64",%"PRIx64"] and [%"PRIx64",%"PRIx64"]\n", > - rmrru->base_address, rmrru->end_address, > - base_addr, end_addr); > + printk(XENLOG_ERR VTDPREFIX "Overlapping RMRRs [%" PRIx64 > + ",%" PRIx64 "] and [%" PRIx64 > + ",%" PRIx64 "]\n", > + rmrru->base_address, rmrru->end_address, base_addr, > + end_addr); These kind of issues with line break could be adjusted using the right penalty coefficients in the clang format config. > > 7. Parentheses for empty functions > ============================================================================== > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -1311,7 +1307,9 @@ void panic(const char *fmt, ...) > -static void cf_check suspend_steal_fn(const char *str, size_t nr) { } > +static void cf_check suspend_steal_fn(const char *str, size_t nr) > +{} I think also this can be mitigated with penalty + a rule saying that empty function can have the brackets on the same line. Cheers, Luca
On 18.02.2025 03:36, Stefano Stabellini wrote: > On Mon, 17 Feb 2025, Jan Beulich wrote: >> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote: >>> 1. Const string arrays reformatting >>> In case the length of items change we might need to introduce a bigger >>> change wrt new formatting of unaffected lines >>> ============================================================================== >>> >>> --- a/xen/drivers/acpi/tables.c >>> +++ b/xen/drivers/acpi/tables.c >>> @@ -38,10 +38,10 @@ >>> -static const char *__initdata >>> -mps_inti_flags_polarity[] = { "dfl", "high", "res", "low" }; >>> -static const char *__initdata >>> -mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" }; >>> +static const char *__initdata mps_inti_flags_polarity[] = { "dfl", "high", >>> + "res", "low" }; >>> +static const char *__initdata mps_inti_flags_trigger[] = { "dfl", "edge", "res", >>> >>> --- a/xen/drivers/acpi/utilities/utglobal.c >>> +++ b/xen/drivers/acpi/utilities/utglobal.c >>> static const char *const acpi_gbl_region_types[ACPI_NUM_PREDEFINED_REGIONS] = { >>> - "SystemMemory", >>> - "SystemIO", >>> - "PCI_Config", >>> - "EmbeddedControl", >>> - "SMBus", >>> - "CMOS", >>> - "PCIBARTarget", >>> - "DataTable" >>> + "SystemMemory", "SystemIO", "PCI_Config", "EmbeddedControl", >>> + "SMBus", "CMOS", "PCIBARTarget", "DataTable" >>> }; >> >> Why in the world would a tool need to touch anything like the two examples >> above? My take is that the code is worse readability-wise afterwards. > > I think the output is acceptable: not necessarily better than before, > but also not significantly worse. Hmm, for the change to xen/drivers/acpi/tables.c I wouldn't agree with this statement. And for xen/drivers/acpi/utilities/utglobal.c remember that this is code taken from ACPI CA, which we may better not re-format. > To me, the main takeaway is that there > are many unavoidable but unnecessary changes. Interesting. I'd say it slightly differently: The main takeaway is that there are many avoidable / unnecessary changes. Jan
On Tue, Feb 18, 2025 at 11:56:48AM +0200, Grygorii Strashko wrote: > Honestly, It looks a bit strange that Xen community is considering batch automated code formatting, > For example Linux kernel cleanly rejected such approach. > Linux kernel docs "4.1.1. Coding style" section [1]. > > Another thing, checking the new code and accepting code style violations (if any) on per-patch basis. That would indeed be my preference, from what I've seen the clang-format based style could live together with the previous style, there doesn't seem to be any irreconcilable style differences that would prevent new and old code chunks from being adjacent. Thanks, Roger.
Hello, Jan, Stefano! On 18.02.25 13:34, Jan Beulich wrote: > On 18.02.2025 03:36, Stefano Stabellini wrote: >> On Mon, 17 Feb 2025, Jan Beulich wrote: >>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote: >>>> 1. Const string arrays reformatting >>>> In case the length of items change we might need to introduce a bigger >>>> change wrt new formatting of unaffected lines >>>> ============================================================================== >>>> >>>> --- a/xen/drivers/acpi/tables.c >>>> +++ b/xen/drivers/acpi/tables.c >>>> @@ -38,10 +38,10 @@ >>>> -static const char *__initdata >>>> -mps_inti_flags_polarity[] = { "dfl", "high", "res", "low" }; >>>> -static const char *__initdata >>>> -mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" }; >>>> +static const char *__initdata mps_inti_flags_polarity[] = { "dfl", "high", >>>> + "res", "low" }; >>>> +static const char *__initdata mps_inti_flags_trigger[] = { "dfl", "edge", "res", >>>> >>>> --- a/xen/drivers/acpi/utilities/utglobal.c >>>> +++ b/xen/drivers/acpi/utilities/utglobal.c >>>> static const char *const acpi_gbl_region_types[ACPI_NUM_PREDEFINED_REGIONS] = { >>>> - "SystemMemory", >>>> - "SystemIO", >>>> - "PCI_Config", >>>> - "EmbeddedControl", >>>> - "SMBus", >>>> - "CMOS", >>>> - "PCIBARTarget", >>>> - "DataTable" >>>> + "SystemMemory", "SystemIO", "PCI_Config", "EmbeddedControl", >>>> + "SMBus", "CMOS", "PCIBARTarget", "DataTable" >>>> }; >>> Why in the world would a tool need to touch anything like the two examples >>> above? My take is that the code is worse readability-wise afterwards. >> I think the output is acceptable: not necessarily better than before, >> but also not significantly worse. > Hmm, for the change to xen/drivers/acpi/tables.c I wouldn't agree with this > statement. And for xen/drivers/acpi/utilities/utglobal.c remember that this > is code taken from ACPI CA, which we may better not re-format. We can use /* clang-format off */ constructs to protect those lines we do not want to be touched by clang-format [1]. This is what Grygprii mentioned in some other e-mail. > >> To me, the main takeaway is that there >> are many unavoidable but unnecessary changes. > Interesting. I'd say it slightly differently: The main takeaway is that > there are many avoidable / unnecessary changes. But at the end of the day it will allow using automatic formatting... > > Jan Thank you, Oleksandr [1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code
On 16/02/2025 5:11 pm, Oleksandr Andrushchenko wrote: > Hello, Roger! > > Please find the branch with all the conversions [1]. > Unfortunately I cannot provide a branch as seen with > diff --ignore-all-space as such a patch will not simply apply. > > Stay safe, > Oleksandr Andrushchenko > > On 16.02.25 13:58, Andrew Cooper wrote: >> On 16/02/2025 10:21 am, Oleksandr Andrushchenko wrote: >>> There are two diff files which show what happens in case the same is >>> applied to the whole xen/drivers directory: >>> - first one is the result of the "git diff" command, 1.2M [3] >>> - the second one is for "git diff --ignire-all-space", 600K [4] >> Please can you format everything, and put it on a branch somewhere, so >> people can browse. >> >> ~Andrew > [1] https://github.com/andr2000/xen/tree/clang_ml_drivers_v002_diff That appears to only be drivers/ Please do *everything*. I want to see what this does to files I consider to be pretty clean Xen-style. ~Andrew
On 19.02.2025 13:43, Oleksandr Andrushchenko wrote: > Hello, Jan, Stefano! > > On 18.02.25 13:34, Jan Beulich wrote: >> On 18.02.2025 03:36, Stefano Stabellini wrote: >>> On Mon, 17 Feb 2025, Jan Beulich wrote: >>>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote: >>>>> 1. Const string arrays reformatting >>>>> In case the length of items change we might need to introduce a bigger >>>>> change wrt new formatting of unaffected lines >>>>> ============================================================================== >>>>> >>>>> --- a/xen/drivers/acpi/tables.c >>>>> +++ b/xen/drivers/acpi/tables.c >>>>> @@ -38,10 +38,10 @@ >>>>> -static const char *__initdata >>>>> -mps_inti_flags_polarity[] = { "dfl", "high", "res", "low" }; >>>>> -static const char *__initdata >>>>> -mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" }; >>>>> +static const char *__initdata mps_inti_flags_polarity[] = { "dfl", "high", >>>>> + "res", "low" }; >>>>> +static const char *__initdata mps_inti_flags_trigger[] = { "dfl", "edge", "res", >>>>> >>>>> --- a/xen/drivers/acpi/utilities/utglobal.c >>>>> +++ b/xen/drivers/acpi/utilities/utglobal.c >>>>> static const char *const acpi_gbl_region_types[ACPI_NUM_PREDEFINED_REGIONS] = { >>>>> - "SystemMemory", >>>>> - "SystemIO", >>>>> - "PCI_Config", >>>>> - "EmbeddedControl", >>>>> - "SMBus", >>>>> - "CMOS", >>>>> - "PCIBARTarget", >>>>> - "DataTable" >>>>> + "SystemMemory", "SystemIO", "PCI_Config", "EmbeddedControl", >>>>> + "SMBus", "CMOS", "PCIBARTarget", "DataTable" >>>>> }; >>>> Why in the world would a tool need to touch anything like the two examples >>>> above? My take is that the code is worse readability-wise afterwards. >>> I think the output is acceptable: not necessarily better than before, >>> but also not significantly worse. >> Hmm, for the change to xen/drivers/acpi/tables.c I wouldn't agree with this >> statement. And for xen/drivers/acpi/utilities/utglobal.c remember that this >> is code taken from ACPI CA, which we may better not re-format. > We can use /* clang-format off */ constructs to protect those lines we > do not want to be touched by clang-format [1]. This is what Grygprii > mentioned in some other e-mail. We have fall-through comments. We have SAF comments. Yet another flavor to keep some external tool happy. If everyone else thinks this is a good idea, I'm not intending to stand in the way. Yet I don't like this as a workaround. Instead I think the tool's going too far. Jan
Hello, Grygorii! On 18.02.25 11:56, Grygorii Strashko wrote: > > > On 16.02.25 12:21, Oleksandr Andrushchenko wrote: >> Hello, everybody! >> >> As agreed before [1] I am sending a series to show two samples of the >> formatting done with clang-format. The clang-format configuration can be >> found at [2]. It already has some notes on previous discussions when >> Luca presented his version of that configuration and which I used to >> start from. >> >> There are two diff files which show what happens in case the same is >> applied to the whole xen/drivers directory: >> - first one is the result of the "git diff" command, 1.2M [3] >> - the second one is for "git diff --ignire-all-space", 600K [4] >> >> This is not to limit the reviewers from seeing a bigger picture and not >> only the files in this series. >> N.B. xen/drivers uses tabs a lot, so this is no surprise the size >> difference is big enough: roughly space conversion is half of the changes. >> >> While reviewing the changes I have collected some of the unexpected things >> done by clang-format and some interesting pieces. You can find those >> below. For some of those I have filed an issue on clang-format and hope the >> community will lead me in resolving those. Of course what I collected is >> not the complete list of such changes, so I hope we can discuss missed >> ones here. >> >> From this exercise I can definitely tell that clang-format does help a >> lot and has potential to be employed as a code formatting tool for Xen. >> Of course it cannot be used as is now and will require discussions on the >> Xen coding style and possibly submitting patches to clang-format to >> satisfy those which cannot be handled by the tool now. >> >> Stay safe, >> Oleksandr Andrushchenko >> >> 1. Const string arrays reformatting >> In case the length of items change we might need to introduce a bigger >> change wrt new formatting of unaffected lines >> ============================================================================== >> >> --- a/xen/drivers/acpi/tables.c >> +++ b/xen/drivers/acpi/tables.c >> @@ -38,10 +38,10 @@ >> -static const char *__initdata >> -mps_inti_flags_polarity[] = { "dfl", "high", "res", "low" }; >> -static const char *__initdata >> -mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" }; >> +static const char *__initdata mps_inti_flags_polarity[] = { "dfl", "high", >> + "res", "low" }; >> +static const char *__initdata mps_inti_flags_trigger[] = { "dfl", "edge", "res", >> >> --- a/xen/drivers/acpi/utilities/utglobal.c >> +++ b/xen/drivers/acpi/utilities/utglobal.c >> static const char *const acpi_gbl_region_types[ACPI_NUM_PREDEFINED_REGIONS] = { >> - "SystemMemory", >> - "SystemIO", >> - "PCI_Config", >> - "EmbeddedControl", >> - "SMBus", >> - "CMOS", >> - "PCIBARTarget", >> - "DataTable" >> + "SystemMemory", "SystemIO", "PCI_Config", "EmbeddedControl", >> + "SMBus", "CMOS", "PCIBARTarget", "DataTable" >> }; >> >> 2. Long strings in ptintk violations >> I filed an issue on printk format strings [5] >> ============================================================================== >> @@ -225,9 +231,9 @@ void __init acpi_table_print_madt_entry(struct acpi_subtable_header *header) >> printk(KERN_DEBUG PREFIX >> - "GIC Distributor (gic_id[0x%04x] address[0x%"PRIx64"] gsi_base[%d])\n", >> - p->gic_id, p->base_address, >> - p->global_irq_base); >> + "GIC Distributor (gic_id[0x%04x] address[0x%" PRIx64 >> + "] gsi_base[%d])\n", >> + p->gic_id, p->base_address, p->global_irq_base); >> >> @@ -629,12 +628,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header) >> - printk(XENLOG_ERR VTDPREFIX >> - "Overlapping RMRRs [%"PRIx64",%"PRIx64"] and [%"PRIx64",%"PRIx64"]\n", >> - rmrru->base_address, rmrru->end_address, >> - base_addr, end_addr); >> + printk(XENLOG_ERR VTDPREFIX "Overlapping RMRRs [%" PRIx64 >> + ",%" PRIx64 "] and [%" PRIx64 >> + ",%" PRIx64 "]\n", >> + rmrru->base_address, rmrru->end_address, base_addr, >> + end_addr); > > It doesn't understand properly things like "PRIx64" :( > > Most of other items, I've mentioned already, >> Unhappy: it's probably "known" things - identification of complex defines and >> static struct/arrays declarations. > > And for them, probably, no magic automation tools exist which will satisfy everybody as, > at least static definitions are usually formatted to achieve better readability > which is always subjective. Strongly agree > > The tags /* clang-format off/clang-format on */ might be useful. > Yes, I guess we will be forced to use those > [..] > > Honestly, It looks a bit strange that Xen community is considering batch automated code formatting, > For example Linux kernel cleanly rejected such approach. > Linux kernel docs "4.1.1. Coding style" section [1]. > > Another thing, checking the new code and accepting code style violations (if any) on per-patch basis. The main difference, and it was brought by Jan before [1], the possible existence of the three different code styles in a single patch: xen, libxl and Linux... Not sure that such a case can be handled. Of course we may require that no such patch should exist, but it does depend on the change being made. Thus, no guarantee > > [1] https://docs.kernel.org/process/4.Coding.html > > BR, > -grygorii [1] https://old-list-archives.xen.org/archives/html/xen-devel/2025-02/msg00464.html
Hello, Andrew! On 19.02.25 14:49, Andrew Cooper wrote: > On 16/02/2025 5:11 pm, Oleksandr Andrushchenko wrote: >> Hello, Roger! >> >> Please find the branch with all the conversions [1]. >> Unfortunately I cannot provide a branch as seen with >> diff --ignore-all-space as such a patch will not simply apply. >> >> Stay safe, >> Oleksandr Andrushchenko >> >> On 16.02.25 13:58, Andrew Cooper wrote: >>> On 16/02/2025 10:21 am, Oleksandr Andrushchenko wrote: >>>> There are two diff files which show what happens in case the same is >>>> applied to the whole xen/drivers directory: >>>> - first one is the result of the "git diff" command, 1.2M [3] >>>> - the second one is for "git diff --ignire-all-space", 600K [4] >>> Please can you format everything, and put it on a branch somewhere, so >>> people can browse. >>> >>> ~Andrew >> [1] https://github.com/andr2000/xen/tree/clang_ml_drivers_v002_diff > That appears to only be drivers/ I thought that was the agreement, so we start from the drivers first > > Please do *everything*. I want to see what this does to files I > consider to be pretty clean Xen-style. Sure > > ~Andrew > Thank you, Oleksandr
Hello, Jan! On 19.02.25 14:49, Jan Beulich wrote: > On 19.02.2025 13:43, Oleksandr Andrushchenko wrote: >> Hello, Jan, Stefano! >> >> On 18.02.25 13:34, Jan Beulich wrote: >>> On 18.02.2025 03:36, Stefano Stabellini wrote: >>>> On Mon, 17 Feb 2025, Jan Beulich wrote: >>>>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote: >>>>>> 1. Const string arrays reformatting >>>>>> In case the length of items change we might need to introduce a bigger >>>>>> change wrt new formatting of unaffected lines >>>>>> ============================================================================== >>>>>> >>>>>> --- a/xen/drivers/acpi/tables.c >>>>>> +++ b/xen/drivers/acpi/tables.c >>>>>> @@ -38,10 +38,10 @@ >>>>>> -static const char *__initdata >>>>>> -mps_inti_flags_polarity[] = { "dfl", "high", "res", "low" }; >>>>>> -static const char *__initdata >>>>>> -mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" }; >>>>>> +static const char *__initdata mps_inti_flags_polarity[] = { "dfl", "high", >>>>>> + "res", "low" }; >>>>>> +static const char *__initdata mps_inti_flags_trigger[] = { "dfl", "edge", "res", >>>>>> >>>>>> --- a/xen/drivers/acpi/utilities/utglobal.c >>>>>> +++ b/xen/drivers/acpi/utilities/utglobal.c >>>>>> static const char *const acpi_gbl_region_types[ACPI_NUM_PREDEFINED_REGIONS] = { >>>>>> - "SystemMemory", >>>>>> - "SystemIO", >>>>>> - "PCI_Config", >>>>>> - "EmbeddedControl", >>>>>> - "SMBus", >>>>>> - "CMOS", >>>>>> - "PCIBARTarget", >>>>>> - "DataTable" >>>>>> + "SystemMemory", "SystemIO", "PCI_Config", "EmbeddedControl", >>>>>> + "SMBus", "CMOS", "PCIBARTarget", "DataTable" >>>>>> }; >>>>> Why in the world would a tool need to touch anything like the two examples >>>>> above? My take is that the code is worse readability-wise afterwards. >>>> I think the output is acceptable: not necessarily better than before, >>>> but also not significantly worse. >>> Hmm, for the change to xen/drivers/acpi/tables.c I wouldn't agree with this >>> statement. And for xen/drivers/acpi/utilities/utglobal.c remember that this >>> is code taken from ACPI CA, which we may better not re-format. >> We can use /* clang-format off */ constructs to protect those lines we >> do not want to be touched by clang-format [1]. This is what Grygprii >> mentioned in some other e-mail. > We have fall-through comments. We have SAF comments. Yet another flavor to > keep some external tool happy. If everyone else thinks this is a good idea, > I'm not intending to stand in the way. Yet I don't like this as a workaround. > Instead I think the tool's going too far. Yes, I do agree. But only if we talk about having an automated code style check now (which is definitely the goal at some time). Before that we could still use the tool to take all that good that it does and manually prepare a set of patches to fix those code style issues which we like. > > Jan
On 19.02.25 14:51, Oleksandr Andrushchenko wrote: > Hello, Andrew! > > On 19.02.25 14:49, Andrew Cooper wrote: >> On 16/02/2025 5:11 pm, Oleksandr Andrushchenko wrote: >>> Hello, Roger! >>> >>> Please find the branch with all the conversions [1]. >>> Unfortunately I cannot provide a branch as seen with >>> diff --ignore-all-space as such a patch will not simply apply. >>> >>> Stay safe, >>> Oleksandr Andrushchenko >>> >>> On 16.02.25 13:58, Andrew Cooper wrote: >>>> On 16/02/2025 10:21 am, Oleksandr Andrushchenko wrote: >>>>> There are two diff files which show what happens in case the same is >>>>> applied to the whole xen/drivers directory: >>>>> - first one is the result of the "git diff" command, 1.2M [3] >>>>> - the second one is for "git diff --ignire-all-space", 600K [4] >>>> Please can you format everything, and put it on a branch somewhere, so >>>> people can browse. >>>> >>>> ~Andrew >>> [1] https://github.com/andr2000/xen/tree/clang_ml_drivers_v002_diff force pushed to the same branch for all Xen >> That appears to only be drivers/ > I thought that was the agreement, so we start from the drivers first >> >> Please do *everything*. I want to see what this does to files I >> consider to be pretty clean Xen-style. > Sure >> >> ~Andrew >> > Thank you, > Oleksandr
On 19/02/2025 1:19 pm, Oleksandr Andrushchenko wrote: > > > On 19.02.25 14:51, Oleksandr Andrushchenko wrote: >> Hello, Andrew! >> >> On 19.02.25 14:49, Andrew Cooper wrote: >>> On 16/02/2025 5:11 pm, Oleksandr Andrushchenko wrote: >>>> Hello, Roger! >>>> >>>> Please find the branch with all the conversions [1]. >>>> Unfortunately I cannot provide a branch as seen with >>>> diff --ignore-all-space as such a patch will not simply apply. >>>> >>>> Stay safe, >>>> Oleksandr Andrushchenko >>>> >>>> On 16.02.25 13:58, Andrew Cooper wrote: >>>>> On 16/02/2025 10:21 am, Oleksandr Andrushchenko wrote: >>>>>> There are two diff files which show what happens in case the same is >>>>>> applied to the whole xen/drivers directory: >>>>>> - first one is the result of the "git diff" command, 1.2M [3] >>>>>> - the second one is for "git diff --ignire-all-space", 600K [4] >>>>> Please can you format everything, and put it on a branch >>>>> somewhere, so >>>>> people can browse. >>>>> >>>>> ~Andrew >>>> [1] https://github.com/andr2000/xen/tree/clang_ml_drivers_v002_diff > force pushed to the same branch for all Xen Thankyou. ~Andrew
--- a/xen/drivers/acpi/tables.c +++ b/xen/drivers/acpi/tables.c @@ -38,10 +38,10 @@ -static const char *__initdata -mps_inti_flags_polarity[] = { "dfl", "high", "res", "low" }; -static const char *__initdata -mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" }; +static const char *__initdata mps_inti_flags_polarity[] = { "dfl", "high", + "res", "low" }; +static const char *__initdata mps_inti_flags_trigger[] = { "dfl", "edge", "res", --- a/xen/drivers/acpi/utilities/utglobal.c +++ b/xen/drivers/acpi/utilities/utglobal.c static const char *const acpi_gbl_region_types[ACPI_NUM_PREDEFINED_REGIONS] = { - "SystemMemory", - "SystemIO", - "PCI_Config", - "EmbeddedControl", - "SMBus", - "CMOS", - "PCIBARTarget", - "DataTable" + "SystemMemory", "SystemIO", "PCI_Config", "EmbeddedControl", + "SMBus", "CMOS", "PCIBARTarget", "DataTable" }; 2. Long strings in ptintk violations I filed an issue on printk format strings [5] ============================================================================== @@ -225,9 +231,9 @@ void __init acpi_table_print_madt_entry(struct acpi_subtable_header *header) printk(KERN_DEBUG PREFIX - "GIC Distributor (gic_id[0x%04x] address[0x%"PRIx64"] gsi_base[%d])\n", - p->gic_id, p->base_address, - p->global_irq_base); + "GIC Distributor (gic_id[0x%04x] address[0x%" PRIx64 + "] gsi_base[%d])\n", + p->gic_id, p->base_address, p->global_irq_base); @@ -629,12 +628,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header) - printk(XENLOG_ERR VTDPREFIX - "Overlapping RMRRs [%"PRIx64",%"PRIx64"] and [%"PRIx64",%"PRIx64"]\n", - rmrru->base_address, rmrru->end_address, - base_addr, end_addr); + printk(XENLOG_ERR VTDPREFIX "Overlapping RMRRs [%" PRIx64 + ",%" PRIx64 "] and [%" PRIx64 + ",%" PRIx64 "]\n", + rmrru->base_address, rmrru->end_address, base_addr, + end_addr); 3. String concatenation after clang - needs manual work to fix ============================================================================== --- a/xen/drivers/acpi/apei/apei-base.c +++ b/xen/drivers/acpi/apei/apei-base.c @@ -171,16 +169,19 @@ int __apei_exec_run(struct apei_exec_context *ctx, u8 action, printk(KERN_WARNING - "Invalid action table, unknown instruction " - "type: %d\n", entry->instruction); + "Invalid action table, unknown instruction " "type: %d\n", + entry->instruction); 4. Looks a bit weird, but correct ============================================================================== --- a/xen/drivers/acpi/apei/apei-io.c +++ b/xen/drivers/acpi/apei/apei-io.c @@ -80,13 +78,15 @@ static void __iomem *__init apei_range_map(paddr_t paddr, unsigned long size) - pg = ((((paddr + size -1) & PAGE_MASK) - - (paddr & PAGE_MASK)) >> PAGE_SHIFT) + 1; + pg = ((((paddr + size - 1) & PAGE_MASK) - (paddr & PAGE_MASK)) >> + PAGE_SHIFT) + + 1; 5. Long line tables mess, think it can be manually reformated before applying clang-format ============================================================================== --- a/xen/drivers/acpi/tables/tbfadt.c +++ b/xen/drivers/acpi/tables/tbfadt.c @@ -74,12 +74,12 @@ typedef struct acpi_fadt_info { - ACPI_FADT_OFFSET(pm1a_event_block), - ACPI_FADT_OFFSET(pm1_event_length), ACPI_FADT_REQUIRED}, + ACPI_FADT_OFFSET(pm1a_event_block), ACPI_FADT_OFFSET(pm1_event_length), + ACPI_FADT_REQUIRED }, --- a/xen/drivers/acpi/utilities/utglobal.c +++ b/xen/drivers/acpi/utilities/utglobal.c @@ -97,71 +96,71 @@ struct acpi_bit_register_info acpi_gbl_bit_register_info[ACPI_NUM_BITREG] = { /* Name Parent Register Register Bit Position Register Bit Mask */ /* ACPI_BITREG_TIMER_STATUS */ { ACPI_REGISTER_PM1_STATUS, - ACPI_BITPOSITION_TIMER_STATUS, - ACPI_BITMASK_TIMER_STATUS}, - /* ACPI_BITREG_BUS_MASTER_STATUS */ {ACPI_REGISTER_PM1_STATUS, - ACPI_BITPOSITION_BUS_MASTER_STATUS, + ACPI_BITPOSITION_TIMER_STATUS, ACPI_BITMASK_TIMER_STATUS }, + /* ACPI_BITREG_BUS_MASTER_STATUS */ + { ACPI_REGISTER_PM1_STATUS, ACPI_BITPOSITION_BUS_MASTER_STATUS, 6. Macro blocks are formatted ============================================================================== Grygorii mentioned this and I was sure it is properly handled, but it is not. I have filed an issue on clang-format [6]. --- a/xen/drivers/char/cadence-uart.c +++ b/xen/drivers/char/cadence-uart.c @@ -189,16 +192,14 @@ static int __init cuart_init(struct dt_device_node *dev, const void *data) DT_DEVICE_START(cuart, "Cadence UART", DEVICE_SERIAL) - .dt_match = cuart_dt_match, - .init = cuart_init, + .dt_match = cuart_dt_match, .init = cuart_init, DT_DEVICE_END 7. Parentheses for empty functions ============================================================================== --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -1311,7 +1307,9 @@ void panic(const char *fmt, ...) -static void cf_check suspend_steal_fn(const char *str, size_t nr) { } +static void cf_check suspend_steal_fn(const char *str, size_t nr) +{} 8. Const struct reformatting is weird and inconsistent ============================================================================== --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -1050,135 +1055,93 @@ static const struct ns16550_config __initconst uart_config[] = .param = param_oxford, }, /* Pericom PI7C9X7951 Uno UART */ - { - .vendor_id = PCI_VENDOR_ID_PERICOM, + { .vendor_id = PCI_VENDOR_ID_PERICOM, .dev_id = 0x7951, - .param = param_pericom_1port - }, + .param = param_pericom_1port }, 9. Define is longer than 80 chars ============================================================================== --- a/xen/drivers/cpufreq/cpufreq_ondemand.c +++ b/xen/drivers/cpufreq/cpufreq_ondemand.c @@ -27,10 +27,8 @@ -#define MIN_STAT_SAMPLING_RATE \ - (MIN_SAMPLING_MILLISECS * MILLISECS(1)) -#define MIN_SAMPLING_RATE \ - (def_sampling_rate / MIN_SAMPLING_RATE_RATIO) +#define MIN_STAT_SAMPLING_RATE (MIN_SAMPLING_MILLISECS * MILLISECS(1)) +#define MIN_SAMPLING_RATE (def_sampling_rate / MIN_SAMPLING_RATE_RATIO) 10. Union memebers require an empty line between them ============================================================================== --- a/xen/drivers/passthrough/amd/iommu-defs.h +++ b/xen/drivers/passthrough/amd/iommu-defs.h @@ -289,6 +289,7 @@ struct amd_iommu_dte { union amd_iommu_control { uint64_t raw; + struct { 11. Multiline string alignment for at the first string, not for the function opening bracket. Depends on the macro before the string? ============================================================================== --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -748,18 +757,18 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn, printk(XENLOG_WARNING "SR-IOV device %pp has its virtual" - " functions already enabled (%04x)\n", &pdev->sbdf, ctrl); + " functions already enabled (%04x)\n", + &pdev->sbdf, ctrl); 11. const struct initializers are inconsistent I have filed a bug on clang-format for that [7] ============================================================================== [snip] static const struct ns16550_config __initconst uart_config[] = { [snip] /* OXPCIe200 1 Native UART */ {
Hello, everybody! As agreed before [1] I am sending a series to show two samples of the formatting done with clang-format. The clang-format configuration can be found at [2]. It already has some notes on previous discussions when Luca presented his version of that configuration and which I used to start from. There are two diff files which show what happens in case the same is applied to the whole xen/drivers directory: - first one is the result of the "git diff" command, 1.2M [3] - the second one is for "git diff --ignire-all-space", 600K [4] This is not to limit the reviewers from seeing a bigger picture and not only the files in this series. N.B. xen/drivers uses tabs a lot, so this is no surprise the size difference is big enough: roughly space conversion is half of the changes. While reviewing the changes I have collected some of the unexpected things done by clang-format and some interesting pieces. You can find those below. For some of those I have filed an issue on clang-format and hope the community will lead me in resolving those. Of course what I collected is not the complete list of such changes, so I hope we can discuss missed ones here. From this exercise I can definitely tell that clang-format does help a lot and has potential to be employed as a code formatting tool for Xen. Of course it cannot be used as is now and will require discussions on the Xen coding style and possibly submitting patches to clang-format to satisfy those which cannot be handled by the tool now. Stay safe, Oleksandr Andrushchenko 1. Const string arrays reformatting In case the length of items change we might need to introduce a bigger change wrt new formatting of unaffected lines ============================================================================== .vendor_id = PCI_VENDOR_ID_OXSEMI, .dev_id = 0xc4cf, .param = param_oxford, }, /* Pericom PI7C9X7951 Uno UART */ { .vendor_id = PCI_VENDOR_ID_PERICOM, .dev_id = 0x7951, .param = param_pericom_1port }, [snip] [1] https://lists.xen.org/archives/html/xen-devel/2025-02/msg00430.html [2] https://github.com/andr2000/xen/blob/clang_ml_drivers_v001/xen/.clang-format [3] https://github.com/andr2000/xen/blob/clang_ml_drivers_v001/xen/drivers.patch [4] https://github.com/andr2000/xen/blob/clang_ml_drivers_v001/xen/drivers_ignore_all_space.patch [5] https://github.com/llvm/llvm-project/issues/127383 [6] https://github.com/llvm/llvm-project/issues/127381 [7] https://github.com/llvm/llvm-project/issues/127380 Oleksandr Andrushchenko (2): code style: Format ns16550 driver code style: Format ACPI tables xen/drivers/acpi/tables.c | 809 ++++++++++++++++++++----------------- xen/drivers/char/ns16550.c | 761 +++++++++++++++++----------------- 2 files changed, 813 insertions(+), 757 deletions(-)