From: Stefano Stabellini <sstabellini@kernel.org>
To: Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com>
Cc: "George.Dunlap@citrix.com" <George.Dunlap@citrix.com>,
"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>,
"jbeulich@suse.com" <jbeulich@suse.com>,
"xen-devel@lists.xenproject.org"
<xen-devel@lists.xenproject.org>
Subject: Re: Xen Coding style and clang-format
Date: Wed, 7 Oct 2020 18:07:44 -0700 (PDT) [thread overview]
Message-ID: <alpine.DEB.2.21.2010071750360.23978@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <3ff3f7d16cdab692178ce638da1a6b880817fb7e.camel@epam.com>
[-- 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 :-)
next prev parent reply other threads:[~2020-10-08 1:08 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 [this message]
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
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=alpine.DEB.2.21.2010071750360.23978@sstabellini-ThinkPad-T480s \
--to=sstabellini@kernel.org \
--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=jbeulich@suse.com \
--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).