* Xen Coding style and clang-format @ 2020-09-30 9:18 Anastasiia Lukianenko 2020-09-30 9:57 ` Jan Beulich 0 siblings, 1 reply; 16+ messages in thread From: Anastasiia Lukianenko @ 2020-09-30 9:18 UTC (permalink / raw) To: xen-devel Cc: Artem Mygaiev, committers, julien, vicooodin, viktor.mitin.19, Volodymyr Babchuk 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Xen Coding style and clang-format 2020-09-30 9:18 Xen Coding style and clang-format Anastasiia Lukianenko @ 2020-09-30 9:57 ` Jan Beulich 2020-09-30 10:24 ` George Dunlap 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2020-09-30 9:57 UTC (permalink / raw) To: Anastasiia Lukianenko Cc: xen-devel, Artem Mygaiev, committers, julien, vicooodin, viktor.mitin.19, Volodymyr Babchuk 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Xen Coding style and clang-format 2020-09-30 9:57 ` Jan Beulich @ 2020-09-30 10:24 ` George Dunlap 2020-10-01 9:06 ` Anastasiia Lukianenko 0 siblings, 1 reply; 16+ messages in thread From: George Dunlap @ 2020-09-30 10:24 UTC (permalink / raw) To: Jan Beulich Cc: Anastasiia Lukianenko, xen-devel, Artem Mygaiev, committers, julien, vicooodin, viktor.mitin.19, Volodymyr Babchuk > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Xen Coding style and clang-format 2020-09-30 10:24 ` George Dunlap @ 2020-10-01 9:06 ` Anastasiia Lukianenko 2020-10-01 10:06 ` George Dunlap 0 siblings, 1 reply; 16+ messages in thread From: Anastasiia Lukianenko @ 2020-10-01 9:06 UTC (permalink / raw) To: jbeulich, George.Dunlap Cc: Artem Mygaiev, committers, julien, vicooodin, xen-devel, viktor.mitin.19, Volodymyr Babchuk 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Xen Coding style and clang-format 2020-10-01 9:06 ` Anastasiia Lukianenko @ 2020-10-01 10:06 ` George Dunlap 2020-10-07 10:19 ` Anastasiia Lukianenko 0 siblings, 1 reply; 16+ messages in thread From: George Dunlap @ 2020-10-01 10:06 UTC (permalink / raw) To: Anastasiia Lukianenko Cc: jbeulich, Artem Mygaiev, committers, julien, vicooodin, xen-devel, viktor.mitin.19, Volodymyr Babchuk > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Xen Coding style and clang-format 2020-10-01 10:06 ` George Dunlap @ 2020-10-07 10:19 ` Anastasiia Lukianenko 2020-10-08 1:07 ` Stefano Stabellini 2020-10-12 18:09 ` George Dunlap 0 siblings, 2 replies; 16+ messages in thread From: Anastasiia Lukianenko @ 2020-10-07 10:19 UTC (permalink / raw) To: George.Dunlap Cc: viktor.mitin.19, vicooodin, julien, Volodymyr Babchuk, Artem Mygaiev, committers, jbeulich, xen-devel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Xen Coding style and clang-format 2020-10-07 10:19 ` Anastasiia Lukianenko @ 2020-10-08 1:07 ` Stefano Stabellini 2020-10-12 14:37 ` Anastasiia Lukianenko 2020-10-12 18:09 ` George Dunlap 1 sibling, 1 reply; 16+ messages in thread From: Stefano Stabellini @ 2020-10-08 1:07 UTC (permalink / raw) To: Anastasiia Lukianenko Cc: George.Dunlap, viktor.mitin.19, vicooodin, julien, Volodymyr Babchuk, Artem Mygaiev, committers, jbeulich, xen-devel [-- 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 :-) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Xen Coding style and clang-format 2020-10-08 1:07 ` Stefano Stabellini @ 2020-10-12 14:37 ` Anastasiia Lukianenko 0 siblings, 0 replies; 16+ messages in thread From: Anastasiia Lukianenko @ 2020-10-12 14:37 UTC (permalink / raw) To: sstabellini Cc: viktor.mitin.19, vicooodin, julien, Volodymyr Babchuk, Artem Mygaiev, committers, George.Dunlap, jbeulich, xen-devel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Xen Coding style and clang-format 2020-10-07 10:19 ` Anastasiia Lukianenko 2020-10-08 1:07 ` Stefano Stabellini @ 2020-10-12 18:09 ` George Dunlap 2020-10-13 12:30 ` Jan Beulich 1 sibling, 1 reply; 16+ messages in thread From: George Dunlap @ 2020-10-12 18:09 UTC (permalink / raw) To: Anastasiia Lukianenko Cc: viktor.mitin.19, vicooodin, julien, Volodymyr Babchuk, Artem Mygaiev, committers, jbeulich, xen-devel > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Xen Coding style and clang-format 2020-10-12 18:09 ` George Dunlap @ 2020-10-13 12:30 ` Jan Beulich 2020-10-16 9:42 ` Anastasiia Lukianenko 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2020-10-13 12:30 UTC (permalink / raw) To: George Dunlap, Anastasiia Lukianenko Cc: viktor.mitin.19, vicooodin, julien, Volodymyr Babchuk, Artem Mygaiev, committers, xen-devel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Xen Coding style and clang-format 2020-10-13 12:30 ` Jan Beulich @ 2020-10-16 9:42 ` Anastasiia Lukianenko 2020-10-16 10:23 ` Julien Grall 0 siblings, 1 reply; 16+ messages in thread From: Anastasiia Lukianenko @ 2020-10-16 9:42 UTC (permalink / raw) To: jbeulich, George.Dunlap Cc: Artem Mygaiev, vicooodin, julien, xen-devel, committers, viktor.mitin.19, Volodymyr Babchuk 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Xen Coding style and clang-format 2020-10-16 9:42 ` Anastasiia Lukianenko @ 2020-10-16 10:23 ` Julien Grall 2020-10-16 11:37 ` Artem Mygaiev 0 siblings, 1 reply; 16+ messages in thread From: Julien Grall @ 2020-10-16 10:23 UTC (permalink / raw) To: Anastasiia Lukianenko, jbeulich, George.Dunlap Cc: Artem Mygaiev, vicooodin, xen-devel, committers, viktor.mitin.19, Volodymyr Babchuk 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: Xen Coding style and clang-format 2020-10-16 10:23 ` Julien Grall @ 2020-10-16 11:37 ` Artem Mygaiev 2020-10-19 18:07 ` Stefano Stabellini 0 siblings, 1 reply; 16+ messages in thread From: Artem Mygaiev @ 2020-10-16 11:37 UTC (permalink / raw) To: Julien Grall, Anastasiia Lukianenko, jbeulich, George.Dunlap Cc: vicooodin, xen-devel, committers, viktor.mitin.19, Volodymyr Babchuk 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: Xen Coding style and clang-format 2020-10-16 11:37 ` Artem Mygaiev @ 2020-10-19 18:07 ` Stefano Stabellini 2020-10-20 17:13 ` Julien Grall 0 siblings, 1 reply; 16+ messages in thread From: Stefano Stabellini @ 2020-10-19 18:07 UTC (permalink / raw) To: Artem Mygaiev Cc: Julien Grall, Anastasiia Lukianenko, jbeulich, George.Dunlap, vicooodin, xen-devel, committers, viktor.mitin.19, Volodymyr Babchuk [-- 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Xen Coding style and clang-format 2020-10-19 18:07 ` Stefano Stabellini @ 2020-10-20 17:13 ` Julien Grall 2020-10-23 9:39 ` Anastasiia Lukianenko 0 siblings, 1 reply; 16+ messages in thread From: Julien Grall @ 2020-10-20 17:13 UTC (permalink / raw) To: Stefano Stabellini, Artem Mygaiev Cc: Anastasiia Lukianenko, jbeulich, George.Dunlap, vicooodin, xen-devel, committers, viktor.mitin.19, Volodymyr Babchuk 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Xen Coding style and clang-format 2020-10-20 17:13 ` Julien Grall @ 2020-10-23 9:39 ` Anastasiia Lukianenko 0 siblings, 0 replies; 16+ messages in thread From: Anastasiia Lukianenko @ 2020-10-23 9:39 UTC (permalink / raw) To: Artem Mygaiev, sstabellini, julien Cc: committers, jbeulich, vicooodin, George.Dunlap, xen-devel, viktor.mitin.19, Volodymyr Babchuk 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-10-23 9:39 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-30 9:18 Xen Coding style and clang-format Anastasiia Lukianenko 2020-09-30 9:57 ` Jan Beulich 2020-09-30 10:24 ` George Dunlap 2020-10-01 9:06 ` Anastasiia Lukianenko 2020-10-01 10:06 ` George Dunlap 2020-10-07 10:19 ` Anastasiia Lukianenko 2020-10-08 1:07 ` Stefano Stabellini 2020-10-12 14:37 ` Anastasiia Lukianenko 2020-10-12 18:09 ` George Dunlap 2020-10-13 12:30 ` Jan Beulich 2020-10-16 9:42 ` Anastasiia Lukianenko 2020-10-16 10:23 ` Julien Grall 2020-10-16 11:37 ` Artem Mygaiev 2020-10-19 18:07 ` Stefano Stabellini 2020-10-20 17:13 ` Julien Grall 2020-10-23 9:39 ` Anastasiia Lukianenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).