Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com>
To: "jbeulich@suse.com" <jbeulich@suse.com>,
	"George.Dunlap@citrix.com" <George.Dunlap@citrix.com>
Cc: Artem Mygaiev <Artem_Mygaiev@epam.com>,
	"vicooodin@gmail.com" <vicooodin@gmail.com>,
	"julien@xen.org" <julien@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"committers@xenproject.org" <committers@xenproject.org>,
	"viktor.mitin.19@gmail.com" <viktor.mitin.19@gmail.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: Xen Coding style and clang-format
Date: Fri, 16 Oct 2020 09:42:39 +0000
Message-ID: <4d4f351b152df2c50e18676ccd6ab6b4dc667801.camel@epam.com> (raw)
In-Reply-To: <b4d7e9a7-6c25-1f7f-86ce-867083beb81a@suse.com>

Hi all,

On Tue, 2020-10-13 at 14:30 +0200, Jan Beulich wrote:
> 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.

If it turns out to make the checker more flexible, then I will try to
add both options as correct.

> > 
> > > 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.

Until I found a way to save the first option, I think this case may
remain in the opinion of the author.

> 
> > > 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.

Fixed this in the new version of checker.

> > 
> > > 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

Thanks for your advices, which helped me improve the checker. I
understand that there are still some disagreements about the
formatting, but as I said before, the checker cannot be very flexible
and take into account all the author's ideas.
I suggest using the
checker not as a mandatory check, but as an indication to the author of
possible formatting errors that he can correct or ignore.

I attached the new version of Xen checker below with updated
clang version from 9.0 to 12.0 and with minor fixes.
(branch xen-clang-format_12)
https://github.com/xen-troops/llvm-project/tree/xen-clang-format_12

If during the using and testing the tool new inconsistencies are found,
I am ready to fix them.

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
2020-10-12 18:09           ` George Dunlap
2020-10-13 12:30             ` Jan Beulich
2020-10-16  9:42               ` Anastasiia Lukianenko [this message]
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=4d4f351b152df2c50e18676ccd6ab6b4dc667801.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=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