Re: Xen Coding style and clang-format

[Anastasiia Lukianenko <Anastasiia_Lukianenko@xxxxxxxx>]

Hi all,

On Thu, 2020-10-01 at 10:06 +0000, George Dunlap wrote:
> > On Oct 1, 2020, at 10:06 AM, Anastasiia Lukianenko <
> > Anastasiia_Lukianenko@xxxxxxxx> wrote:
> > 
> > Hi,
> > 
> > On Wed, 2020-09-30 at 10:24 +0000, George Dunlap wrote:
> > > > On Sep 30, 2020, at 10:57 AM, Jan Beulich <jbeulich@xxxxxxxx>
> > > > wrote:
> > > > 
> > > > On 30.09.2020 11:18, Anastasiia Lukianenko wrote:
> > > > > I would like to know your opinion on the following coding
> > > > > style
> > > > > cases.
> > > > > Which option do you think is correct?
> > > > > 1) Function prototype when the string length is longer than
> > > > > the
> > > > > allowed
> > > > > one
> > > > > -static int __init
> > > > > -acpi_parse_gic_cpu_interface(struct acpi_subtable_header
> > > > > *header,
> > > > > -                             const unsigned long end)
> > > > > +static int __init acpi_parse_gic_cpu_interface(
> > > > > +    struct acpi_subtable_header *header, const unsigned long
> > > > > end)
> > > > 
> > > > Both variants are deemed valid style, I think (same also goes
> > > > for
> > > > function calls with this same problem). In fact you mix two
> > > > different style aspects together (placement of parameter
> > > > declarations and placement of return type etc) - for each
> > > > individually both forms are deemed acceptable, I think.
> > > 
> > > If we’re going to have a tool go through and report (correct?)
> > > all
> > > these coding style things, it’s an opportunity to think if we
> > > want to
> > > add new coding style requirements (or change existing
> > > requirements).
> > > 
> > 
> > I am ready to discuss new requirements and implement them in rules
> > of
> > the Xen Coding style checker.
> 
> Thank you. :-)  But what I meant was: Right now we don’t require one
> approach or the other for this specific instance.  Do we want to
> choose one?
> 
> I think in this case it makes sense to do the easiest thing.  If it’s
> easy to make the current tool accept both styles, let’s just do that
> for now.  If the tool currently forces you to choose one of the two
> styles, let’s choose one.
> 
>  -George

During the detailed study of the Xen checker and the Clang-Format Style
Options, it was found that this tool, unfortunately, is not so flexible
to allow the author to independently choose the formatting style in
situations that I described in the last letter. For example define code
style:
-#define ALLREGS \
-    C(r0, r0_usr);   C(r1, r1_usr);   C(r2, r2_usr);   C(r3,
r3_usr);   \
-    C(cpsr, cpsr)
+#define ALLREGS            \
+    C(r0, r0_usr);         \
+    C(r1, r1_usr);         \
+    C(r2, r2_usr);         \
There are also some inconsistencies in the formatting of the tool and
what is written in the hyung coding style rules. For example, the
comment format:
-    /* PC should be always a multiple of 4, as Xen is using ARM
instruction set */
+    /* PC should be always a multiple of 4, as Xen is using ARM
instruction set
+     */
I would like to draw your attention to the fact that the comment
behaves in this way, since the line length exceeds the allowable one.
The ReflowComments option is responsible for this format. It can be
turned off, but then the result will be:
ReflowComments=false:
/* second veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongComment with
plenty of information */

ReflowComments=true:
/* second veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongComment with
plenty of
 * information */

So I want to know if the community is ready to add new formatting
options and edit old ones. Below I will give examples of what
corrections the checker is currently making (the first variant in each
case is existing code and the second variant is formatted by checker).
If they fit the standards, then I can document them in the coding
style. If not, then I try to configure the checker. But the idea is
that we need to choose one option that will be considered correct.
1) Function prototype when the string length is longer than the allowed
-static int __init
-acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
-                             const unsigned long end)
+static int __init acpi_parse_gic_cpu_interface(
+    struct acpi_subtable_header *header, const unsigned long end)
2) Wrapping an operation to a new line when the string length is longer
than the allowed
-    status = acpi_get_table(ACPI_SIG_SPCR, 0,
-                            (struct acpi_table_header **)&spcr);
+    status =
+        acpi_get_table(ACPI_SIG_SPCR, 0, (struct acpi_table_header
**)&spcr);
3) Space after brackets
-    return ((char *) base + offset);
+    return ((char *)base + offset);
4) Spaces in brackets in switch condition
-    switch ( domctl->cmd )
+    switch (domctl->cmd)
5) Spaces in brackets in operation
-    imm = ( insn >> BRANCH_INSN_IMM_SHIFT ) & BRANCH_INSN_IMM_MASK;
+    imm = (insn >> BRANCH_INSN_IMM_SHIFT) & BRANCH_INSN_IMM_MASK;
6) Spaces in brackets in return
-        return ( !sym->name[2] || sym->name[2] == '.' );
+        return (!sym->name[2] || sym->name[2] == '.');
7) Space after sizeof
-    clean_and_invalidate_dcache_va_range(new_ptr, sizeof (*new_ptr) *
len);
+    clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr) *
len);
8) Spaces before comment if it’s on the same line
-    case R_ARM_MOVT_ABS: /* S + A */
+    case R_ARM_MOVT_ABS:    /* S + A */

-    if ( tmp == 0UL )       /* Are any bits set? */
-        return result + size;   /* Nope. */
+    if ( tmp == 0UL )         /* Are any bits set? */
+        return result + size; /* Nope. */

9) Space after for_each_vcpu
-        for_each_vcpu(d, v)
+        for_each_vcpu (d, v)
10) Spaces in declaration
-    union hsr hsr = { .bits = regs->hsr };
+    union hsr hsr = {.bits = regs->hsr};

Regards,
Anastasiia