From: Jan Beulich <jbeulich@suse.com>
To: George Dunlap <George.Dunlap@citrix.com>,
Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com>
Cc: "viktor.mitin.19@gmail.com" <viktor.mitin.19@gmail.com>,
"vicooodin@gmail.com" <vicooodin@gmail.com>,
"julien@xen.org" <julien@xen.org>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
Artem Mygaiev <Artem_Mygaiev@epam.com>,
"committers@xenproject.org" <committers@xenproject.org>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: Xen Coding style and clang-format
Date: Tue, 13 Oct 2020 14:30:02 +0200 [thread overview]
Message-ID: <b4d7e9a7-6c25-1f7f-86ce-867083beb81a@suse.com> (raw)
In-Reply-To: <64FE5ADB-2359-4A31-B1A1-925750515D98@citrix.com>
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
next prev parent reply other threads:[~2020-10-13 12:30 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b4d7e9a7-6c25-1f7f-86ce-867083beb81a@suse.com \
--to=jbeulich@suse.com \
--cc=Anastasiia_Lukianenko@epam.com \
--cc=Artem_Mygaiev@epam.com \
--cc=George.Dunlap@citrix.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=committers@xenproject.org \
--cc=julien@xen.org \
--cc=vicooodin@gmail.com \
--cc=viktor.mitin.19@gmail.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).