Coccinelle Archive on lore.kernel.org
 help / color / Atom feed
From: Joe Perches <joe@perches.com>
To: Julia Lawall <julia.lawall@inria.fr>
Cc: Giuseppe Scrivano <gscrivan@redhat.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Julia Lawall <julia.lawall@lip6.fr>,
	Andy Whitcroft <apw@shadowen.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	cocci <cocci@systeme.lip6.fr>
Subject: Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
Date: Thu, 20 Aug 2020 09:52:41 -0700
Message-ID: <a5713d8597065ef986f780499428fcc4cd31c003.camel@perches.com> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2008201021400.2524@hadrien>

On Thu, 2020-08-20 at 10:33 +0200, Julia Lawall wrote:
> On Wed, 19 Aug 2020, Joe Perches wrote:
> > On Wed, 2020-08-19 at 14:22 -0700, Joe Perches wrote:
> > > There are commas used as statement terminations that should typically
> > > have used semicolons instead.  Only direct assignments or use of a single
> > > function or value on a single line are detected by this test.
> > > 
> > > e.g.:
> > > 	foo = bar(),		/* typical use is semicolon not comma */
> > > 	bar = baz();
> > > 
> > > Add an imperfect test to detect these comma uses.
> > > 
> > > No false positives were found in testing, but many types of false negatives
> > > are possible.
> > > 
> > > e.g.:
> > > 	foo = bar() + 1,	/* comma use, but not direct assignment */
> > > 	bar = baz();
> > 
> > Hi.
> > 
> > I recently added a test for this condition to linux's checkpatch.
> > 
> > A similar coccinelle script might be:
> > 
> > $ cat comma.cocci
> > @@
> > expression e1;
> > expression e2;
> > @@
> > 
> > 	e1
> > -	,
> > +	;
> > 	e2;
> > $
> > 
> > This works reasonably well but it has several false positives
> > for declarations like:
> > 
> > $ spatch --sp-file comma.cocci mm/huge_memory.c
> > diff -u -p a/huge_memory.c b/huge_memory.c
> > --- a/huge_memory.c
> > +++ b/huge_memory.c
> > @@ -2778,7 +2778,7 @@ static unsigned long deferred_split_scan
> >  	struct pglist_data *pgdata = NODE_DATA(sc->nid);
> >  	struct deferred_split *ds_queue = &pgdata->deferred_split_queue;
> >  	unsigned long flags;
> > -	LIST_HEAD(list), *pos, *next;
> > +	LIST_HEAD(list), *pos; *next;
> >  	struct page *page;
> >  	int split = 0;
> > $
> > 
> > Any script improvement suggestions?
> 
> I have a bunch of variations of this that are more complicated than I
> would have expected.  One shorter variant that I have is:
> 
> @@
> expression e1,e2;
> statement S;
> @@
> 
>  S
>  e1
> -,
> +;
>   (<+... e2 ...+>);
> 
> This will miss cases where the first statement is the comma thing.  But I
> think it is possible to improve this now.  I will check.

Hi Julia.

Right, thanks, this adds a dependency on a statement
before the expression.  Any stragglers would be easily
found using slightly different form.
There are not very many of these in linux kernel.

Another nicety would be to allow the s/,/;/ conversion to
find both b and c in this sequence:
	a = 1;
	b = 2,
	c = 3,
	d = 4;
without running the script multiple times.
There are many dozen uses of this style in linux kernel.

I tried variants of adding a comma after the e2 expression,
but cocci seems to have parsing problems with:

@@
expression e1;
expression e2;
@@
	e1
-	,
+	;
	e2,

I do appreciate that coccinelle adds braces for multiple
expression comma use after an if.

i.e.:
	if (foo)
		a = 1, b = 2;
becomes
	if (foo) {
		a = 1; b = 2;
	}

There are a few dozen uses of this style in linux kernel.


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

  reply index

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20200818184107.f8af232fb58b17160c570874@linux-foundation.org>
     [not found] ` <3bf27caf462007dfa75647b040ab3191374a59de.camel@perches.com>
2020-08-19 23:07   ` Joe Perches
2020-08-20  8:33     ` Julia Lawall
2020-08-20 16:52       ` Joe Perches [this message]
2020-08-20 17:03         ` Julia Lawall
2020-08-20 17:28           ` Joe Perches
2020-08-22  1:08           ` Joe Perches
2020-08-22  3:35             ` Valdis Klētnieks
2020-08-22  5:30               ` Joe Perches
2020-08-22  7:07                 ` Julia Lawall
2020-09-24 20:19                   ` Thomas Gleixner
2020-09-24 20:21                     ` Julia Lawall
2020-09-24 20:33                     ` Joe Perches
2020-09-24 21:53                       ` Thomas Gleixner
2020-09-24 22:23                         ` Joe Perches
2020-09-25 17:06                           ` Julia Lawall
2020-09-25 17:26                             ` Joe Perches
2020-09-26 19:11                               ` Valdis Klētnieks
2020-09-27 17:08                           ` Julia Lawall
2020-09-27 17:45                             ` Joe Perches
2020-09-27 19:35                               ` Julia Lawall

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=a5713d8597065ef986f780499428fcc4cd31c003.camel@perches.com \
    --to=joe@perches.com \
    --cc=akpm@linux-foundation.org \
    --cc=apw@shadowen.org \
    --cc=cocci@systeme.lip6.fr \
    --cc=gscrivan@redhat.com \
    --cc=julia.lawall@inria.fr \
    --cc=julia.lawall@lip6.fr \
    --cc=linux-kernel@vger.kernel.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

Coccinelle Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/cocci/0 cocci/git/0.git

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

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/fr.lip6.systeme.cocci


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