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