Coccinelle Archive on lore.kernel.org
 help / color / Atom feed
From: Julia Lawall <julia.lawall@inria.fr>
To: Denis Efremov <efremov@linux.com>
Cc: cocci@systeme.lip6.fr, linux-kernel@vger.kernel.org
Subject: Re: [Cocci] [PATCH] coccinelle: misc: add swap script
Date: Thu, 18 Feb 2021 12:29:02 +0100 (CET)
Message-ID: <alpine.DEB.2.22.394.2102181227230.2748@hadrien> (raw)
In-Reply-To: <4e913cc4-cb86-4552-bced-a89cbecca3b2@linux.com>



On Thu, 18 Feb 2021, Denis Efremov wrote:

>
>
> On 2/18/21 1:17 PM, Julia Lawall wrote:
> >
> >
> > On Thu, 18 Feb 2021, Denis Efremov wrote:
> >
> >>
> >>
> >> On 2/18/21 12:31 AM, Julia Lawall wrote:
> >>>> +@depends on patch@
> >>>> +identifier tmp;
> >>>> +expression a, b;
> >>>> +type T;
> >>>> +@@
> >>>> +
> >>>> +(
> >>>> +- T tmp;
> >>>> +|
> >>>> +- T tmp = 0;
> >>>> +|
> >>>> +- T *tmp = NULL;
> >>>> +)
> >>>> +... when != tmp
> >>>> +- tmp = a;
> >>>> +- a = b;
> >>>> +- b = tmp;
> >>>> ++ swap(a, b);
> >>>> +... when != tmp
> >>>
> >>> In this rule and the next one, if you remove the final ; from the b = tmp
> >>> line and from the swap line, and put it into context code afterwards, them
> >>> the generated code looks better on cases like fs/xfs/xfs_inode.c in the
> >>> function xfs_lock_two_inodes where two successive swap calls are
> >>> generated.
> >>>
> >>> There are also some cases such as drivers/net/wireless/ath/ath5k/phy.c in
> >>> the function ath5k_hw_get_median_noise_floor where the swap code makes up
> >>> a whole if branch.
> >>
> >>> In this cases it would be good to remove the {}.
> >>
> >> How this can be handled?
> >>
> >> If I use this pattern:
> >> @depends on patch@
> >> identifier tmp;
> >> expression a, b;
> >> @@
> >>
> >> (
> >>   if (...)
> >> - {
> >> -       tmp = a;
> >> -       a = b;
> >> -       b = tmp
> >> +       swap(a, b)
> >>         ;
> >> - }
> >> |
> >> -       tmp = a;
> >> -       a = b;
> >> -       b = tmp
> >> +       swap(a, b)
> >>         ;
> >> )
> >>
> >> The tool fails with error:
> >>
> >> EXN: Failure("rule starting on line 58: already tagged token:\nC code
> >> context\nFile \"drivers/net/wireless/ath/ath5k/phy.c\", line 1574,
> >> column 4, charpos = 41650\n around = 'sort',\n whole content =
> >> \t\t\t\tsort[j - 1] = tmp;") in drivers/net/wireless/ath/ath5k/phy.c
> >
> > A disjunction basically says "at this node in the cfg, can I match the
> > first patter, or can I match the second pattern, etc."  Unfortunately in
> > this case the two branches start matching at different nodes, so the short
> > circuit aspect of a disjunction isn't used, and it matches both patterns.
> >
> > The solution is to just make two rules.  The first for the if case and the
> > second for everything else.
> >
>
>   if (...)
> - {
> -       tmp = a;
> -       a = b;
> -       b = tmp
> +       swap(a, b)
>         ;
> - }
>
>
> This produces "single-line ifs".
> Maybe generated patches can be improved somehow?
> Moving -+; doesn't help in this case.

There is clearly some problem with the management of newlines...

The other alternative is to just have one rule for introducing swap and
another for removing the braces around a swap, ie

if (...)
- {
  swap(...);
- }

I don't think it would be motivated to remove the newline in that case.

julia

>
> diff -u -p a/drivers/net/wireless/ath/ath9k/calib.c b/drivers/net/wireless/ath/ath9k/calib.c
> --- a/drivers/net/wireless/ath/ath9k/calib.c
> +++ b/drivers/net/wireless/ath/ath9k/calib.c
> @@ -32,11 +32,7 @@ static int16_t ath9k_hw_get_nf_hist_mid(
>
>         for (i = 0; i < ATH9K_NF_CAL_HIST_MAX - 1; i++) {
>                 for (j = 1; j < ATH9K_NF_CAL_HIST_MAX - i; j++) {
> -                       if (sort[j] > sort[j - 1]) {
> -                               nfval = sort[j];
> -                               sort[j] = sort[j - 1];
> -                               sort[j - 1] = nfval;
> -                       }
> +                       if (sort[j] > sort[j - 1]) swap(sort[j], sort[j - 1]);
>                 }
>         }
>         nfval = sort[(ATH9K_NF_CAL_HIST_MAX - 1) >> 1];
> diff -u -p a/drivers/net/wireless/ath/ath9k/ar9003_calib.c b/drivers/net/wireless/ath/ath9k/ar9003_calib.c
> --- a/drivers/net/wireless/ath/ath9k/ar9003_calib.c
> +++ b/drivers/net/wireless/ath/ath9k/ar9003_calib.c
> @@ -1011,19 +1011,11 @@ static void __ar955x_tx_iq_cal_sort(stru
>                 for (ix = 0; ix < MAXIQCAL - 1; ix++) {
>                         for (iy = ix + 1; iy <= MAXIQCAL - 1; iy++) {
>                                 if (coeff->mag_coeff[i][im][iy] <
> -                                   coeff->mag_coeff[i][im][ix]) {
> -                                       temp = coeff->mag_coeff[i][im][ix];
> -                                       coeff->mag_coeff[i][im][ix] =
> -                                               coeff->mag_coeff[i][im][iy];
> -                                       coeff->mag_coeff[i][im][iy] = temp;
> -                               }
> +                                   coeff->mag_coeff[i][im][ix]) swap(coeff->mag_coeff[i][im][ix],
> +                                                                     coeff->mag_coeff[i][im][iy]);
>                                 if (coeff->phs_coeff[i][im][iy] <
> -                                   coeff->phs_coeff[i][im][ix]) {
> -                                       temp = coeff->phs_coeff[i][im][ix];
> -                                       coeff->phs_coeff[i][im][ix] =
> -                                               coeff->phs_coeff[i][im][iy];
> -                                       coeff->phs_coeff[i][im][iy] = temp;
> -                               }
> +                                   coeff->phs_coeff[i][im][ix]) swap(coeff->phs_coeff[i][im][ix],
> +                                                                     coeff->phs_coeff[i][im][iy]);
>
> Thanks,
> Denis
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-16  8:01 Denis Efremov
2021-02-16 18:05 ` Markus Elfring
2021-02-17 21:31 ` Julia Lawall
2021-02-18  6:13   ` Denis Efremov
2021-02-18 10:17     ` Julia Lawall
2021-02-18 11:03       ` Denis Efremov
2021-02-18 11:29         ` Julia Lawall [this message]
2021-02-18 11:34           ` Denis Efremov
     [not found]       ` <50eb8319-a552-c749-6143-7e24a8778a04@web.de>
2021-02-18 20:03         ` Julia Lawall
2021-02-19  9:05 ` [Cocci] [PATCH v2] coccinelle: misc: add minmax script Denis Efremov
2021-02-19  9:07   ` Denis Efremov
2021-02-19  9:24 ` [Cocci] [PATCH v2] coccinelle: misc: add swap script Denis Efremov
2021-03-03 16:37   ` Julia Lawall
2021-03-05 10:09 ` [Cocci] [PATCH v3] " Denis Efremov
     [not found]   ` <afb06bbc-5c28-8a92-f205-c9a1c87c707c@linux.com>
2021-04-04 12:33     ` 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=alpine.DEB.2.22.394.2102181227230.2748@hadrien \
    --to=julia.lawall@inria.fr \
    --cc=cocci@systeme.lip6.fr \
    --cc=efremov@linux.com \
    --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