Mailing List Archive

1 2  View All
Re: Xen Coding style and clang-format [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
> 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 [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
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 [ In reply to ]
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

1 2  View All