Re: [PATCH 0/2] code style exercise: Drivers folder samples

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

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