Re: Xen Coding style and clang-format

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.10.2020 20:09, George Dunlap wrote:
>> On Oct 7, 2020, at 11:19 AM, Anastasiia Lukianenko 
>> <Anastasiia_Lukianenko@xxxxxxxx> 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