On Fri, 5 Jun 2015, Fabian Frederick wrote: > > > > On 31 May 2015 at 11:42 Julia Lawall <julia.law...@lip6.fr> 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);
There is no connection between tmp and swap here. Tmp does have the same name as some variable used in a previous swap, but there is no guarantee that this swap here is the one that was introduced by the previous rule. Unfortunately, when you make a transformation, all information about positions is lost. So there is no way to put a position variable on a generated swap and ensure that the one matched here is exactly the same. Maybe you could go a little closer to ensuring that property by inheriting i1 and i2 as well, and not just tmp. julia > > @@ > 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); >
