Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Anastasiia Lukianenko <Anastasiia_Lukianenko@epam.com>
To: "George.Dunlap@citrix.com" <George.Dunlap@citrix.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>,
	"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 10:19:27 +0000
Message-ID: <3ff3f7d16cdab692178ce638da1a6b880817fb7e.camel@epam.com> (raw)
In-Reply-To: <B64C5E67-7BEA-4C31-9089-AB8CC1F1E80F@citrix.com>

Hi all,

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

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

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 [this message]
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
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=3ff3f7d16cdab692178ce638da1a6b880817fb7e.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