Re: [PATCH 1/1] scripts/coccinelle/misc: add swap.cocci

[Fabian Frederick]

> On 31 May 2015 at 11:42 Julia Lawall <julia.lawall@xxxxxxx> wrote:
>
>
> I propose the extended version below (not currently coccicheck friendly).Â
> the extra features are:
>
> 1. The original version requires i1 and i2 to be identifiers, eg x and y.Â
> This doesn't address the case where they are terms line x->a or x[b]. The
> fact that the original code contains assignments with both i1 and i2 on
> the left hand side is enough to ensure that they are appropriate arguments
> for swap. So they can be changed to expression metavariables.
>
> 2. The original patch rule always removed the tmp variable. This is not
> valid if the tmp variable is used for something else. The new semantic
> patch separates the introduction of swap (rule r) from the removal of the
> variable declaration (rule ok and the one folowing). The rule ok checks
> that this is a function containing an introduced call to swap, and then
> the rule after removes the declaration if the variable is not used for
> anything else. Note that the name of the tmp variable is remembered in
> the invalid three-argument version of sawp. This is then cleaned up in
> the rule below.
>
> 3. The original patch always removed the initialization of the tmp
> variable. Actually, some code uses the tmp variable afterwards to refer
> to the old value. In the new semantic patch, the first set of rules
> considers the cases where the tmp variable is not used, and the last rule
> is for the case where the tmp variable is stll needed. No cleaning up of
> the declaration is needed in that case.
>
> There is one regression as compared to the original semantic patch: In the
> file lib/mpi/mpi-pow.c, the temporary variable is not needed after the
> change, but it is also not removed. It is declared within a loop, and
> Coccinelle does not realize that it is not needed afterwards, because it
> is needed on subsequent loop iterations. Trying to adjust the semantic
> patch to address this issue made it much slower and didn't fix the
> problem. Perhaps it is easier to rely on gcc to give an unused variable
> warning, and to clean it up then.
>
> Fabian, if you are o with this, do you want to sgenify it ans submit a new
> patch?

Hello Julia,

    Interesting improvements :) Do we really need tmp variable in swap() ?
I tried the version below which removes swap(x,y,tmp)->swap(x,y) rule
and had the same result on drivers branch plus it solved a missing tmp.
Maybe you want to avoid out of context swap() calls ?

Regards,
Fabian

@r@
expression i1, i2, E;
identifier tmp;
@@

- tmp = i1;
- i1 = i2;
- i2 = tmp;
+ swap(i1, i2);
 ... when != tmp
? tmp = E

@ok exists@
type t1;
identifier r.tmp;
expression i1,i2;
position p;
@@

t1@p tmp;
...
swap(i1, i2);

@@
expression i1,i2;
identifier tmp;
type t1;
position ok.p;
@@

-t1@p tmp;
Â<... when strict
   when != tmp
Âswap(i1, i2);
Â...>


// tmp variable still needed

@@
expression i1, i2;
identifier tmp;
@@

 tmp = i1;
- i1 = i2;
- i2 = tmp;
+ swap(i1, i2);


>
> thanks,
> julia
>
> // it may be possible to remove the tmp variable
>
> @r@
> expression i1, i2, E;
> identifier tmp;
> @@
>
> - tmp = i1;
> - i1 = i2;
> - i2 = tmp;
> + swap(i1, i2, tmp);
>Â Â... when != tmp
> ? tmp = E
>
> @ok exists@
> type t1;
> identifier r.tmp;
> expression i1,i2;
> position p;
> @@
>
> t1@p tmp;
> ...
> swap(i1, i2, tmp);
>
> @@
> expression i1,i2;
> identifier tmp;
> type t1;
> position ok.p;
> @@
>
> -t1@p tmp;
>Â <... when strict
>Â Â Â Âwhen != tmp
>Â swap(i1, i2, tmp);
>Â ...>
>
> @depends on r@
> expression i1,i2;
> identifier tmp;
> @@
>
> swap(i1,i2
> - ,tmp
>Â )
>
> // tmp variable still needed
>
> @@
> expression i1, i2;
> identifier tmp;
> @@
>
>Â Âtmp = i1;
> - i1 = i2;
> - i2 = tmp;
> + swap(i1, i2);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/