[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Xen Coding style and clang-format


  • To: "sstabellini@xxxxxxxxxx" <sstabellini@xxxxxxxxxx>
  • From: Anastasiia Lukianenko <Anastasiia_Lukianenko@xxxxxxxx>
  • Date: Mon, 12 Oct 2020 14:37:58 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=hX3IZ+BX9MUB9mUwmLhy1WSZc8H8kV7aXxWovIdiUf4=; b=OopyCgnRCQ5fggPNMuCIP0D2mkjxay6bePUzcgqQWypQYEuhJ2s7GckVAzsUS4aXZaYhq5NJ3nk9VCVjERmIn31yR+iPIOQtQLgzdNs4bf3CySdOhN9Ive82+z3XsO1rOZo5wVcgmcJn+gDP6a5jqKn8quESuhJVMuGUtTEKnEYPviyWMHsB7PANaCtTPkzz6Vlt9fJpFmdyJRh10EdkyfWX/fAgprsCoYf9yj6k89sdHgn/EjNXPOgbX62d+dYTqdELqf7ECJUnnUPeYjRGYgjgpFonnvW4ZxGcEYZ1xaJeXjLajzqWsPNECNQW+xLr35vx8ReE7VHKcz/hkIe/GQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=itiyESxEjlTiJPNUa0OkRLxBsz3gq31G322uoyWowW8zKpp1ifN1ufAyYvKi8ez3aok87DVsT9TWDY1IdxYGJbtPIEmXStdD9gfXvu2/ru7uPZK4cqLp/yI1C2U0epBygcFlGZlXZNiduSd4avFTDTiDlV/EhrtKrnCsLFmhlwJomt8tL+RZaNsGJ7p8mh90ZEueWPEECw1kSvCqnZPpRKmc8H6NGXNxgnmE5PpuSHiO7F+z0DX4y3FgeloSlsnnhzRftaSTVsd4X3glDDlEQg205RzP1vjj1hKxN1Xep0in4TIHIegJMt+3pnirMSp6xqNYNY9pQ6RuPUYf+H+78Q==
  • Authentication-results: kernel.org; dkim=none (message not signed) header.d=none;kernel.org; dmarc=none action=none header.from=epam.com;
  • Cc: "viktor.mitin.19@xxxxxxxxx" <viktor.mitin.19@xxxxxxxxx>, "vicooodin@xxxxxxxxx" <vicooodin@xxxxxxxxx>, "julien@xxxxxxx" <julien@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Artem Mygaiev <Artem_Mygaiev@xxxxxxxx>, "committers@xxxxxxxxxxxxxx" <committers@xxxxxxxxxxxxxx>, "George.Dunlap@xxxxxxxxxx" <George.Dunlap@xxxxxxxxxx>, "jbeulich@xxxxxxxx" <jbeulich@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 12 Oct 2020 14:38:10 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHWlwq4nKYEhMN38U+xmvwRsutq+amA8joAgAAHUgCAAXyKAIAAENkAgAlxpgCAAPgwAIAHK7QA
  • Thread-topic: Xen Coding style and clang-format

Hi all,

On Wed, 2020-10-07 at 18:07 -0700, Stefano Stabellini wrote:
> 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@xxxxxxxx> 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@xxxxxxxx>
> > > > > > 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 :-)

Thank you for comments :)

If no one objects, I will soon prepare a version of the checker with
minor changes and additions to the coding style document.

Regards,
Anastasiia

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.