Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com>
To: "sstabellini@kernel.org" <sstabellini@kernel.org>
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>,
	"George.Dunlap@citrix.com" <George.Dunlap@citrix.com>,
	"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: Mon, 12 Oct 2020 14:37:58 +0000
Message-ID: <c391abc7b03b90496dea307ef9a8a08d94a862a9.camel@epam.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2010071750360.23978@sstabellini-ThinkPad-T480s>

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@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 :-)

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

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-30  9:18 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 [this message]
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=c391abc7b03b90496dea307ef9a8a08d94a862a9.camel@epam.com \
    --to=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=sstabellini@kernel.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

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git