Xen Coding style and clang-format

Xen Coding style and clang-format

[Anastasiia Lukianenko]

Hi all,

I would like to bring up a discussion about a Xen-checker tool, the
development of which has been temporarily suspended. The idea of this
tool is to use the clang-format approach as a base for Xen ‘checkpatch’
process. The new tool consists of modified clang-format binary and
modified clang-format-diff.py python script to automate Xen patches
format checking and reformatting. The tool can be used as a pre-commit
hook to check and format every patch automatically.
Xen checker is currently in a state of testing, but there are
controversial points regarding the coding style that I would like to
discuss with the community and make a unanimous decision on the
correctness of the formatting.

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)
2) Wrapping an operator to a new line when the length of the line is
longer than the allowed one
-    if ( table->revision > 6
-         || (table->revision == 6 && fadt->minor_revision >= 0) )
+    if ( table->revision > 6 ||
+         (table->revision == 6 && fadt->minor_revision >= 0) )
3) 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);         \
4) Comment style
-    /* 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
+     */

Please find below the repo created by Viktor Mitin with xen clang-
format under the next link (branch xen-clang-format):
https://github.com/xen-troops/llvm-project/tree/xen-clang-format

The next script can be used as an example of how to build clang-format:

https://github.com/viktor-mitin/xen-clang-format-example/blob/master/build_clang_format.sh

Regards,
Anastasiia Lukianenko

Re: Xen Coding style and clang-format

[Jan Beulich]

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.

> 2) Wrapping an operator to a new line when the length of the line is
> longer than the allowed one
> -    if ( table->revision > 6
> -         || (table->revision == 6 && fadt->minor_revision >= 0) )
> +    if ( table->revision > 6 ||
> +         (table->revision == 6 && fadt->minor_revision >= 0) )

Only the latter variant is correct.

> 3) 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);         \

You're again mixing multiple style aspects together: There definitely
should be a blank after the comma, but I don't think we require
every item to be on its own line. But this latter aspect is a little
bogus here anyway - generally, macros better wouldn't contain
embedded semicolons, unless e.g. in a compound statement.

I also don't think we require backslashes (not) to be aligned; this
has typically been left to the author's taste so far, I guess.

> 4) Comment style
> -    /* 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
> +     */

For a single line comment, only the former variant is correct.
In a multi-line comment neither would be. But comment style is
described well in ./CODING_STYLE, I think, so I'm not sure why
the question arose in the first place.

Jan

Re: Xen Coding style and clang-format

[George Dunlap]

> On Sep 30, 2020, at 10:57 AM, Jan Beulich <jbeulich@suse.com> 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).

 -George

Re: Xen Coding style and clang-format

[Anastasiia Lukianenko]

Hi,

On Wed, 2020-09-30 at 10:24 +0000, George Dunlap wrote:
> > On Sep 30, 2020, at 10:57 AM, Jan Beulich <jbeulich@suse.com>
> > 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.

>  -George

Regards,
Anastasiia

Re: Xen Coding style and clang-format

[George Dunlap]

> On Oct 1, 2020, at 10:06 AM, Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com> 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@suse.com>
>>> 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

Re: Xen Coding style and clang-format

[Anastasiia Lukianenko]

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@epam.com> 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@suse.com>
> > > > 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

Re: Xen Coding style and clang-format

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 7005 bytes --]

On Wed, 7 Oct 2020, Anastasiia Lukianenko wrote:
> On Thu, 2020-10-01 at 10:06 +0000, George Dunlap wrote:
> > > On Oct 1, 2020, at 10:06 AM, Anastasiia Lukianenko <
> > > Anastasiia_Lukianenko@epam.com> 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@suse.com>
> > > > > 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 */

To me, the principal goal of the tool is to identify code style
violations. Suggesting how to fix a violation is an added bonus but not
strictly necessary.

So, I think we definitely want the tool to report the following line as
an error, because the line is too long:

/* second veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongComment with plenty of information */

The suggestion on how to fix it is less important. Do we need to set
ReflowComments=true if we want to the tool to report the line as
erroneous? I take that the answer is "yes"?


> 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};

None of these points are particularly problematic to me. I think that
some of them are good to have anyway, like 3) and 8). Some others are
not great, in particular 1) and 2), and I would prefer to keep the
current coding style for those, but I'd be certainly happy to make those
changes anyway if we get a good code style checker in exchange :-)

Re: Xen Coding style and clang-format

[Anastasiia Lukianenko]

Hi all,

On Wed, 2020-10-07 at 18:07 -0700, Stefano Stabellini wrote:
> On Wed, 7 Oct 2020, Anastasiia Lukianenko wrote:
> > On Thu, 2020-10-01 at 10:06 +0000, George Dunlap wrote:
> > > > On Oct 1, 2020, at 10:06 AM, Anastasiia Lukianenko <
> > > > Anastasiia_Lukianenko@epam.com> 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@suse.com>
> > > > > > 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 */
> 
> To me, the principal goal of the tool is to identify code style
> violations. Suggesting how to fix a violation is an added bonus but
> not
> strictly necessary.
> 
> So, I think we definitely want the tool to report the following line
> as
> an error, because the line is too long:
> 
> /* second veryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLongComment
> with plenty of information */
> 
> The suggestion on how to fix it is less important. Do we need to set
> ReflowComments=true if we want to the tool to report the line as
> erroneous? I take that the answer is "yes"?
> 
> 
> > 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};
> 
> None of these points are particularly problematic to me. I think that
> some of them are good to have anyway, like 3) and 8). Some others are
> not great, in particular 1) and 2), and I would prefer to keep the
> current coding style for those, but I'd be certainly happy to make
> those
> changes anyway if we get a good code style checker in exchange :-)

Thank you for comments :)

If no one objects, I will soon prepare a version of the checker with
minor changes and additions to the coding style document.

Regards,
Anastasiia

Re: Xen Coding style and clang-format

[George Dunlap]

> On Oct 7, 2020, at 11:19 AM, Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com> wrote:
> 
> 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@epam.com> 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@suse.com>
>>>>> 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)

Jan already commented on this one; is there any way to tell the checker to ignore  this discrepancy?

If not, I think we should just choose one; I’d go with the latter.

> 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);

Personally I prefer the first version.

> 3) Space after brackets
> -    return ((char *) base + offset);
> +    return ((char *)base + offset);

This seems like a good change to me.

> 4) Spaces in brackets in switch condition
> -    switch ( domctl->cmd )
> +    switch (domctl->cmd)

This is explicitly against the current coding style.

> 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;

I *think* this is already the official style.

> 6) Spaces in brackets in return
> -        return ( !sym->name[2] || sym->name[2] == '.' );
> +        return (!sym->name[2] || sym->name[2] == '.');

Similarly, I think this is already the official style.

> 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);

I think this is correct.

> 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. */

Seem OK to me.

> 
> 9) Space after for_each_vcpu
> -        for_each_vcpu(d, v)
> +        for_each_vcpu (d, v)

Er, not sure about this one.  This is actually a macro; but obviously it looks like for ( ).

I think Jan will probably have an opinion, and I think he’ll be back tomorrow; so maybe wait just a day or two before starting to prep your series.

> 10) Spaces in declaration
> -    union hsr hsr = { .bits = regs->hsr };
> +    union hsr hsr = {.bits = regs->hsr};

I’m fine with this too.

 -George

Re: Xen Coding style and clang-format

[Jan Beulich]

On 12.10.2020 20:09, George Dunlap wrote:
>> On Oct 7, 2020, at 11:19 AM, Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com> wrote:
>> 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)
> 
> Jan already commented on this one; is there any way to tell the checker to ignore  this discrepancy?
> 
> If not, I think we should just choose one; I’d go with the latter.
> 
>> 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);
> 
> Personally I prefer the first version.

Same here.

>> 3) Space after brackets
>> -    return ((char *) base + offset);
>> +    return ((char *)base + offset);
> 
> This seems like a good change to me.
> 
>> 4) Spaces in brackets in switch condition
>> -    switch ( domctl->cmd )
>> +    switch (domctl->cmd)
> 
> This is explicitly against the current coding style.
> 
>> 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;
> 
> I *think* this is already the official style.
> 
>> 6) Spaces in brackets in return
>> -        return ( !sym->name[2] || sym->name[2] == '.' );
>> +        return (!sym->name[2] || sym->name[2] == '.');
> 
> Similarly, I think this is already the official style.
> 
>> 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);
> 
> I think this is correct.

I agree with George on all of the above.

>> 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. */
> 
> Seem OK to me.

I don't think we have any rules how far apart a comment needs
to be; I don't think there should be any complaints or
"corrections" here.

>> 9) Space after for_each_vcpu
>> -        for_each_vcpu(d, v)
>> +        for_each_vcpu (d, v)
> 
> Er, not sure about this one.  This is actually a macro; but obviously it looks like for ( ).
> 
> I think Jan will probably have an opinion, and I think he’ll be back tomorrow; so maybe wait just a day or two before starting to prep your series.

This makes it look like Linux style. In Xen it ought to be one
of

        for_each_vcpu(d, v)
        for_each_vcpu ( d, v )

depending on whether the author of a change considers
for_each_vcpu an ordinary identifier or kind of a keyword.

>> 10) Spaces in declaration
>> -    union hsr hsr = { .bits = regs->hsr };
>> +    union hsr hsr = {.bits = regs->hsr};
> 
> I’m fine with this too.

I think we commonly put the blanks there that are being suggested
to get dropped, so I'm not convinced this is a change we would
want the tool making or suggesting.

Jan

Re: Xen Coding style and clang-format

[Anastasiia Lukianenko]

Hi all,

On Tue, 2020-10-13 at 14:30 +0200, Jan Beulich wrote:
> On 12.10.2020 20:09, George Dunlap wrote:
> > > On Oct 7, 2020, at 11:19 AM, Anastasiia Lukianenko <
> > > Anastasiia_Lukianenko@epam.com> wrote:
> > > 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)
> > 
> > Jan already commented on this one; is there any way to tell the
> > checker to ignore  this discrepancy?
> > 
> > If not, I think we should just choose one; I’d go with the latter.

If it turns out to make the checker more flexible, then I will try to
add both options as correct.

> > 
> > > 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);
> > 
> > Personally I prefer the first version.
> 
> Same here.

Until I found a way to save the first option, I think this case may
remain in the opinion of the author.

> 
> > > 3) Space after brackets
> > > -    return ((char *) base + offset);
> > > +    return ((char *)base + offset);
> > 
> > This seems like a good change to me.
> > 
> > > 4) Spaces in brackets in switch condition
> > > -    switch ( domctl->cmd )
> > > +    switch (domctl->cmd)
> > 
> > This is explicitly against the current coding style.

Fixed this in the new version of checker.

> > 
> > > 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;
> > 
> > I *think* this is already the official style.
> > 
> > > 6) Spaces in brackets in return
> > > -        return ( !sym->name[2] || sym->name[2] == '.' );
> > > +        return (!sym->name[2] || sym->name[2] == '.');
> > 
> > Similarly, I think this is already the official style.
> > 
> > > 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);
> > 
> > I think this is correct.
> 
> I agree with George on all of the above.
> 
> > > 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. */
> > 
> > Seem OK to me.
> 
> I don't think we have any rules how far apart a comment needs
> to be; I don't think there should be any complaints or
> "corrections" here.
> 
> > > 9) Space after for_each_vcpu
> > > -        for_each_vcpu(d, v)
> > > +        for_each_vcpu (d, v)
> > 
> > Er, not sure about this one.  This is actually a macro; but
> > obviously it looks like for ( ).
> > 
> > I think Jan will probably have an opinion, and I think he’ll be
> > back tomorrow; so maybe wait just a day or two before starting to
> > prep your series.
> 
> This makes it look like Linux style. In Xen it ought to be one
> of
> 
>         for_each_vcpu(d, v)
>         for_each_vcpu ( d, v )
> 
> depending on whether the author of a change considers
> for_each_vcpu an ordinary identifier or kind of a keyword.
> 
> > > 10) Spaces in declaration
> > > -    union hsr hsr = { .bits = regs->hsr };
> > > +    union hsr hsr = {.bits = regs->hsr};
> > 
> > I’m fine with this too.
> 
> I think we commonly put the blanks there that are being suggested
> to get dropped, so I'm not convinced this is a change we would
> want the tool making or suggesting.
> 
> Jan

Thanks for your advices, which helped me improve the checker. I
understand that there are still some disagreements about the
formatting, but as I said before, the checker cannot be very flexible
and take into account all the author's ideas.
I suggest using the
checker not as a mandatory check, but as an indication to the author of
possible formatting errors that he can correct or ignore.

I attached the new version of Xen checker below with updated
clang version from 9.0 to 12.0 and with minor fixes.
(branch xen-clang-format_12)
https://github.com/xen-troops/llvm-project/tree/xen-clang-format_12

If during the using and testing the tool new inconsistencies are found,
I am ready to fix them.

Regards,
Anastasiia

Re: Xen Coding style and clang-format

[Julien Grall]

Hi,

On 16/10/2020 10:42, Anastasiia Lukianenko wrote:
> Thanks for your advices, which helped me improve the checker. I
> understand that there are still some disagreements about the
> formatting, but as I said before, the checker cannot be very flexible
> and take into account all the author's ideas.

I am not sure what you refer by "author's ideas" here. The checker 
should follow a coding style (Xen or a modified version):
    - Anything not following the coding style should be considered as 
invalid.
    - Anything not written in the coding style should be left 
untouched/uncommented by the checker.

> I suggest using the
> checker not as a mandatory check, but as an indication to the author of
> possible formatting errors that he can correct or ignore.

I can understand that short term we would want to make it optional so 
either the coding style or the checker can be tuned. But I don't think 
this is an ideal situation to be in long term.

The goal of the checker is to automatically verify the coding style and 
get it consistent across Xen. If we make it optional or it is 
"unreliable", then we lose the two benefits and possibly increase the 
contributor frustration as the checker would say A but we need B.

Therefore, we need to make sure the checker and the coding style match. 
I don't have any opinions on the approach to achieve that.

Cheers,

-- 
Julien Grall

RE: Xen Coding style and clang-format

[Artem Mygaiev]

Hi,

-----Original Message-----
From: Julien Grall <julien@xen.org> 
Sent: пятница, 16 октября 2020 г. 13:24
To: Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com>; jbeulich@suse.com; George.Dunlap@citrix.com
Cc: Artem Mygaiev <Artem_Mygaiev@epam.com>; vicooodin@gmail.com; xen-devel@lists.xenproject.org; committers@xenproject.org; viktor.mitin.19@gmail.com; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: Xen Coding style and clang-format

> Hi,
>
> On 16/10/2020 10:42, Anastasiia Lukianenko wrote:
> > Thanks for your advices, which helped me improve the checker. I
> > understand that there are still some disagreements about the
> > formatting, but as I said before, the checker cannot be very flexible
> > and take into account all the author's ideas.
>
> I am not sure what you refer by "author's ideas" here. The checker 
> should follow a coding style (Xen or a modified version):
>     - Anything not following the coding style should be considered as 
> invalid.
>     - Anything not written in the coding style should be left 
> untouched/uncommented by the checker.
>

Agree

> > I suggest using the
> > checker not as a mandatory check, but as an indication to the author of
> > possible formatting errors that he can correct or ignore.
>
> I can understand that short term we would want to make it optional so 
> either the coding style or the checker can be tuned. But I don't think 
> this is an ideal situation to be in long term.
>
> The goal of the checker is to automatically verify the coding style and 
> get it consistent across Xen. If we make it optional or it is 
> "unreliable", then we lose the two benefits and possibly increase the 
> contributor frustration as the checker would say A but we need B.
>
> Therefore, we need to make sure the checker and the coding style match. 
> I don't have any opinions on the approach to achieve that.

Of the list of remaining issues from Anastasiia, looks like only items 5
and 6 are conform to official Xen coding style. As for remainning ones,
I would like to suggest disabling those that are controversial (items 1,
2, 4, 8, 9, 10). Maybe we want to have further discussion on refining 
coding style, we can use these as starting point. If we are open to
extending style now, I would suggest to add rules that seem to be
meaningful (items 3, 7) and keep them in checker.

-- Artem

RE: Xen Coding style and clang-format

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2689 bytes --]

On Fri, 16 Oct 2020, Artem Mygaiev wrote:
> -----Original Message-----
> From: Julien Grall <julien@xen.org> 
> Sent: пятница, 16 октября 2020 г. 13:24
> To: Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com>; jbeulich@suse.com; George.Dunlap@citrix.com
> Cc: Artem Mygaiev <Artem_Mygaiev@epam.com>; vicooodin@gmail.com; xen-devel@lists.xenproject.org; committers@xenproject.org; viktor.mitin.19@gmail.com; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> Subject: Re: Xen Coding style and clang-format
> 
> > Hi,
> >
> > On 16/10/2020 10:42, Anastasiia Lukianenko wrote:
> > > Thanks for your advices, which helped me improve the checker. I
> > > understand that there are still some disagreements about the
> > > formatting, but as I said before, the checker cannot be very flexible
> > > and take into account all the author's ideas.
> >
> > I am not sure what you refer by "author's ideas" here. The checker 
> > should follow a coding style (Xen or a modified version):
> >     - Anything not following the coding style should be considered as 
> > invalid.
> >     - Anything not written in the coding style should be left 
> > untouched/uncommented by the checker.
> >
> 
> Agree
> 
> > > I suggest using the
> > > checker not as a mandatory check, but as an indication to the author of
> > > possible formatting errors that he can correct or ignore.
> >
> > I can understand that short term we would want to make it optional so 
> > either the coding style or the checker can be tuned. But I don't think 
> > this is an ideal situation to be in long term.
> >
> > The goal of the checker is to automatically verify the coding style and 
> > get it consistent across Xen. If we make it optional or it is 
> > "unreliable", then we lose the two benefits and possibly increase the 
> > contributor frustration as the checker would say A but we need B.
> >
> > Therefore, we need to make sure the checker and the coding style match. 
> > I don't have any opinions on the approach to achieve that.
> 
> Of the list of remaining issues from Anastasiia, looks like only items 5
> and 6 are conform to official Xen coding style. As for remainning ones,
> I would like to suggest disabling those that are controversial (items 1,
> 2, 4, 8, 9, 10). Maybe we want to have further discussion on refining 
> coding style, we can use these as starting point. If we are open to
> extending style now, I would suggest to add rules that seem to be
> meaningful (items 3, 7) and keep them in checker.

Good approach. Yes, I would like to keep 3, 7 in the checker.

I would also keep 8 and add a small note to the coding style to say that
comments should be aligned where possible.

Re: Xen Coding style and clang-format

[Julien Grall]

Hi,

On 19/10/2020 19:07, Stefano Stabellini wrote:
> On Fri, 16 Oct 2020, Artem Mygaiev wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: пятница, 16 октября 2020 г. 13:24
>> To: Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com>; jbeulich@suse.com; George.Dunlap@citrix.com
>> Cc: Artem Mygaiev <Artem_Mygaiev@epam.com>; vicooodin@gmail.com; xen-devel@lists.xenproject.org; committers@xenproject.org; viktor.mitin.19@gmail.com; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> Subject: Re: Xen Coding style and clang-format
>>
>>> Hi,
>>>
>>> On 16/10/2020 10:42, Anastasiia Lukianenko wrote:
>>>> Thanks for your advices, which helped me improve the checker. I
>>>> understand that there are still some disagreements about the
>>>> formatting, but as I said before, the checker cannot be very flexible
>>>> and take into account all the author's ideas.
>>>
>>> I am not sure what you refer by "author's ideas" here. The checker
>>> should follow a coding style (Xen or a modified version):
>>>      - Anything not following the coding style should be considered as
>>> invalid.
>>>      - Anything not written in the coding style should be left
>>> untouched/uncommented by the checker.
>>>
>>
>> Agree
>>
>>>> I suggest using the
>>>> checker not as a mandatory check, but as an indication to the author of
>>>> possible formatting errors that he can correct or ignore.
>>>
>>> I can understand that short term we would want to make it optional so
>>> either the coding style or the checker can be tuned. But I don't think
>>> this is an ideal situation to be in long term.
>>>
>>> The goal of the checker is to automatically verify the coding style and
>>> get it consistent across Xen. If we make it optional or it is
>>> "unreliable", then we lose the two benefits and possibly increase the
>>> contributor frustration as the checker would say A but we need B.
>>>
>>> Therefore, we need to make sure the checker and the coding style match.
>>> I don't have any opinions on the approach to achieve that.
>>
>> Of the list of remaining issues from Anastasiia, looks like only items 5
>> and 6 are conform to official Xen coding style. As for remainning ones,
>> I would like to suggest disabling those that are controversial (items 1,
>> 2, 4, 8, 9, 10). Maybe we want to have further discussion on refining
>> coding style, we can use these as starting point. If we are open to
>> extending style now, I would suggest to add rules that seem to be
>> meaningful (items 3, 7) and keep them in checker.
> 
> Good approach. Yes, I would like to keep 3, 7 in the checker.
> 
> I would also keep 8 and add a small note to the coding style to say that
> comments should be aligned where possible.

+1 for this. Although, I don't mind the coding style used as long as we 
have a checker and the code is consistent :).

Cheers,

-- 
Julien Grall

Re: Xen Coding style and clang-format

[Anastasiia Lukianenko]

Hi all,

On Tue, 2020-10-20 at 18:13 +0100, Julien Grall wrote:
> Hi,
> 
> On 19/10/2020 19:07, Stefano Stabellini wrote:
> > On Fri, 16 Oct 2020, Artem Mygaiev wrote:
> > > -----Original Message-----
> > > From: Julien Grall <julien@xen.org>
> > > Sent: пятница, 16 октября 2020 г. 13:24
> > > To: Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com>; 
> > > jbeulich@suse.com; George.Dunlap@citrix.com
> > > Cc: Artem Mygaiev <Artem_Mygaiev@epam.com>; vicooodin@gmail.com; 
> > > xen-devel@lists.xenproject.org; committers@xenproject.org; 
> > > viktor.mitin.19@gmail.com; Volodymyr Babchuk <
> > > Volodymyr_Babchuk@epam.com>
> > > Subject: Re: Xen Coding style and clang-format
> > > 
> > > > Hi,
> > > > 
> > > > On 16/10/2020 10:42, Anastasiia Lukianenko wrote:
> > > > > Thanks for your advices, which helped me improve the checker.
> > > > > I
> > > > > understand that there are still some disagreements about the
> > > > > formatting, but as I said before, the checker cannot be very
> > > > > flexible
> > > > > and take into account all the author's ideas.
> > > > 
> > > > I am not sure what you refer by "author's ideas" here. The
> > > > checker
> > > > should follow a coding style (Xen or a modified version):
> > > >      - Anything not following the coding style should be
> > > > considered as
> > > > invalid.
> > > >      - Anything not written in the coding style should be left
> > > > untouched/uncommented by the checker.
> > > > 
> > > 
> > > Agree
> > > 
> > > > > I suggest using the
> > > > > checker not as a mandatory check, but as an indication to the
> > > > > author of
> > > > > possible formatting errors that he can correct or ignore.
> > > > 
> > > > I can understand that short term we would want to make it
> > > > optional so
> > > > either the coding style or the checker can be tuned. But I
> > > > don't think
> > > > this is an ideal situation to be in long term.
> > > > 
> > > > The goal of the checker is to automatically verify the coding
> > > > style and
> > > > get it consistent across Xen. If we make it optional or it is
> > > > "unreliable", then we lose the two benefits and possibly
> > > > increase the
> > > > contributor frustration as the checker would say A but we need
> > > > B.
> > > > 
> > > > Therefore, we need to make sure the checker and the coding
> > > > style match.
> > > > I don't have any opinions on the approach to achieve that.
> > > 
> > > Of the list of remaining issues from Anastasiia, looks like only
> > > items 5
> > > and 6 are conform to official Xen coding style. As for remainning
> > > ones,
> > > I would like to suggest disabling those that are controversial
> > > (items 1,
> > > 2, 4, 8, 9, 10). Maybe we want to have further discussion on
> > > refining
> > > coding style, we can use these as starting point. If we are open
> > > to
> > > extending style now, I would suggest to add rules that seem to be
> > > meaningful (items 3, 7) and keep them in checker.
> > 
> > Good approach. Yes, I would like to keep 3, 7 in the checker.
> > 
> > I would also keep 8 and add a small note to the coding style to say
> > that
> > comments should be aligned where possible.
> 
> +1 for this. Although, I don't mind the coding style used as long as
> we 
> have a checker and the code is consistent :).
> 
> Cheers,
> 
Thank you for advices :)
Now I'm trying to figure out the option that needs to be corrected for
the checker to work correctly:
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);
As it turned out, this case is quite rare and the rule for transferring
parameters works correctly in other cases:
-    status = acpi_get_table(ACPI_SIG_SPCR, 0, &spcr, ACPI_SIG_SPC, 0,
ACPI_SIG_SP, 0);
+    status = acpi_get_table(ACPI_SIG_SPCR, 0, &spcr, ACPI_SIG_SPC, 0,
+                            ACPI_SIG_SP, 0);
Thus the checker does not work correctly in the case when the prototype
parameter starts with a parenthesis. I'm going to ask clang community
is this behavior is expected or maybe it's a bug.

Regards,
Anastasiia