> On Oct 7, 2020, at 11:19 AM, Anastasiia Lukianenko
> <anastasiia_lukiane...@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_lukiane...@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 <jbeul...@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
