Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time

Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time

[Julia Lawall]

On Wed, 21 Dec 2016, Guenter Roeck wrote:

> Hi Julia,
>
> On Wed, Dec 21, 2016 at 08:39:38PM +0100, Julia Lawall wrote:
> >
> >
> > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> >
> > > Hi Julia,
> > >
> > > On Wed, Dec 21, 2016 at 03:05:37PM +0100, Julia Lawall wrote:
> > > > A solution is below: the semantic patch, an explanation of the semantic
> > > > patch, and the results.  I have only tried to compile the results (make
> > > > drivers/hwmon/).  Two affected files were not considered for compilation:
> > > >
> > > > drivers/hwmon/vexpress-hwmon.o
> > > > drivers/hwmon/jz4740-hwmon.o
>
> I compile tested those two patches. If possible please drop vexpress-hwmon.c
> from the patch series; the changes in that file don't add any value.
>
> I compile tested all files, and reviewed the patch. It all looks good.
> Please submit the series.
>
> Again, thanks a lot for your help!

I have sent the patches.  I adjusted the semantic patch so that the
indentation of function parameters/arguments would only change if the
length of the function name changes.

Do you think this could be of more general interest in the Linux kernel?
Since the semantic patch works pretty well, I could add it to the
scripts/coccinelle directory?  Previously, however, I got some negative
feedback about this change, because people felt that the new names hid the
actual behavior, so I didn't pursue it.

julia

Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time

[Guenter Roeck]

On 12/22/2016 04:29 AM, Julia Lawall wrote:
>
>
> On Wed, 21 Dec 2016, Guenter Roeck wrote:
>
>> Hi Julia,
>>
>> On Wed, Dec 21, 2016 at 08:39:38PM +0100, Julia Lawall wrote:
>>>
>>>
>>> On Wed, 21 Dec 2016, Guenter Roeck wrote:
>>>
>>>> Hi Julia,
>>>>
>>>> On Wed, Dec 21, 2016 at 03:05:37PM +0100, Julia Lawall wrote:
>>>>> A solution is below: the semantic patch, an explanation of the semantic
>>>>> patch, and the results.  I have only tried to compile the results (make
>>>>> drivers/hwmon/).  Two affected files were not considered for compilation:
>>>>>
>>>>> drivers/hwmon/vexpress-hwmon.o
>>>>> drivers/hwmon/jz4740-hwmon.o
>>
>> I compile tested those two patches. If possible please drop vexpress-hwmon.c
>> from the patch series; the changes in that file don't add any value.
>>
>> I compile tested all files, and reviewed the patch. It all looks good.
>> Please submit the series.
>>
>> Again, thanks a lot for your help!
>
> I have sent the patches.  I adjusted the semantic patch so that the
> indentation of function parameters/arguments would only change if the
> length of the function name changes.
>
> Do you think this could be of more general interest in the Linux kernel?
> Since the semantic patch works pretty well, I could add it to the
> scripts/coccinelle directory?  Previously, however, I got some negative
> feedback about this change, because people felt that the new names hid the
> actual behavior, so I didn't pursue it.
>

I do think it would add a lot of value, if for nothing else as an excellent example
of what can be done with coccinelle.

I actually liked the name changes. I think it is a good idea if the function name
reflects the sysfs attribute it serves (isn't that exactly what it does, ie its
behavior ?). But, as you have experienced, some people inadvertently did not like
it. Given that, I am not sure if it is worth adding it to the kernel source tree.
Maybe you could submit it as RFC so it is at least on record.

Anyway, for SENSOR_DEVICE_ATTR(), I'll have to be a bit more flexible since
the function _will_ be reused. I'll need something like
	SENSOR_DEVICE_ATTR_{RO,RW,WO}(attr, func, param)

Maybe Greg would be open to something like DEVICE_ATTR_FUNC_{RO,RW,WO}(attr,func)
to accommodate the "I want my own function name" crowd ? That would also solve
the case where the function is reused for multiple attributes.

Thanks,
Guenter

Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time

[Julia Lawall]

On Thu, 22 Dec 2016, Guenter Roeck wrote:

> On 12/22/2016 04:29 AM, Julia Lawall wrote:
> >
> >
> > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> >
> > > Hi Julia,
> > >
> > > On Wed, Dec 21, 2016 at 08:39:38PM +0100, Julia Lawall wrote:
> > > >
> > > >
> > > > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> > > >
> > > > > Hi Julia,
> > > > >
> > > > > On Wed, Dec 21, 2016 at 03:05:37PM +0100, Julia Lawall wrote:
> > > > > > A solution is below: the semantic patch, an explanation of the
> > > > > > semantic
> > > > > > patch, and the results.  I have only tried to compile the results
> > > > > > (make
> > > > > > drivers/hwmon/).  Two affected files were not considered for
> > > > > > compilation:
> > > > > >
> > > > > > drivers/hwmon/vexpress-hwmon.o
> > > > > > drivers/hwmon/jz4740-hwmon.o
> > >
> > > I compile tested those two patches. If possible please drop
> > > vexpress-hwmon.c
> > > from the patch series; the changes in that file don't add any value.
> > >
> > > I compile tested all files, and reviewed the patch. It all looks good.
> > > Please submit the series.
> > >
> > > Again, thanks a lot for your help!
> >
> > I have sent the patches.  I adjusted the semantic patch so that the
> > indentation of function parameters/arguments would only change if the
> > length of the function name changes.
> >
> > Do you think this could be of more general interest in the Linux kernel?
> > Since the semantic patch works pretty well, I could add it to the
> > scripts/coccinelle directory?  Previously, however, I got some negative
> > feedback about this change, because people felt that the new names hid the
> > actual behavior, so I didn't pursue it.
> >
>
> I do think it would add a lot of value, if for nothing else as an excellent
> example
> of what can be done with coccinelle.
>
> I actually liked the name changes. I think it is a good idea if the function
> name
> reflects the sysfs attribute it serves (isn't that exactly what it does, ie
> its
> behavior ?). But, as you have experienced, some people inadvertently did not
> like
> it. Given that, I am not sure if it is worth adding it to the kernel source
> tree.
> Maybe you could submit it as RFC so it is at least on record.
>
> Anyway, for SENSOR_DEVICE_ATTR(), I'll have to be a bit more flexible since
> the function _will_ be reused. I'll need something like
> 	SENSOR_DEVICE_ATTR_{RO,RW,WO}(attr, func, param)

Chosen at random,

static SENSOR_DEVICE_ATTR_2(sf2_point4_fan1, S_IRUGO | S_IWUSR,
                        show_sf2_point, store_sf2_point, 4, 1);

should become

static SENSOR_DEVICE_ATTR_2_RW(sf2_point4_fan1, sf2_point, 4, 1); ?

And the functions should be renamed with show and store at the end?

> Maybe Greg would be open to something like
> DEVICE_ATTR_FUNC_{RO,RW,WO}(attr,func)
> to accommodate the "I want my own function name" crowd ? That would also solve
> the case where the function is reused for multiple attributes.

Actually, it was the DEVICE_ATTR_{RO,RW,WO} that wasn't liked.  It doesn't
show the exact permission numbers.  The fact that not all DEVICE_ATTR uses
can be changed due to function reuse is awkward, though.  Greg, do you
have any thoughts about that?

Currently, there are around 1100 calls to DEVICE_ATTR_{RO,RW,WO}.

julia

Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time

[Greg KH]

On Thu, Dec 22, 2016 at 04:45:45PM +0100, Julia Lawall wrote:
> 
> 
> On Thu, 22 Dec 2016, Guenter Roeck wrote:
> 
> > On 12/22/2016 04:29 AM, Julia Lawall wrote:
> > >
> > >
> > > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> > >
> > > > Hi Julia,
> > > >
> > > > On Wed, Dec 21, 2016 at 08:39:38PM +0100, Julia Lawall wrote:
> > > > >
> > > > >
> > > > > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> > > > >
> > > > > > Hi Julia,
> > > > > >
> > > > > > On Wed, Dec 21, 2016 at 03:05:37PM +0100, Julia Lawall wrote:
> > > > > > > A solution is below: the semantic patch, an explanation of the
> > > > > > > semantic
> > > > > > > patch, and the results.  I have only tried to compile the results
> > > > > > > (make
> > > > > > > drivers/hwmon/).  Two affected files were not considered for
> > > > > > > compilation:
> > > > > > >
> > > > > > > drivers/hwmon/vexpress-hwmon.o
> > > > > > > drivers/hwmon/jz4740-hwmon.o
> > > >
> > > > I compile tested those two patches. If possible please drop
> > > > vexpress-hwmon.c
> > > > from the patch series; the changes in that file don't add any value.
> > > >
> > > > I compile tested all files, and reviewed the patch. It all looks good.
> > > > Please submit the series.
> > > >
> > > > Again, thanks a lot for your help!
> > >
> > > I have sent the patches.  I adjusted the semantic patch so that the
> > > indentation of function parameters/arguments would only change if the
> > > length of the function name changes.
> > >
> > > Do you think this could be of more general interest in the Linux kernel?
> > > Since the semantic patch works pretty well, I could add it to the
> > > scripts/coccinelle directory?  Previously, however, I got some negative
> > > feedback about this change, because people felt that the new names hid the
> > > actual behavior, so I didn't pursue it.
> > >
> >
> > I do think it would add a lot of value, if for nothing else as an excellent
> > example
> > of what can be done with coccinelle.
> >
> > I actually liked the name changes. I think it is a good idea if the function
> > name
> > reflects the sysfs attribute it serves (isn't that exactly what it does, ie
> > its
> > behavior ?). But, as you have experienced, some people inadvertently did not
> > like
> > it. Given that, I am not sure if it is worth adding it to the kernel source
> > tree.
> > Maybe you could submit it as RFC so it is at least on record.
> >
> > Anyway, for SENSOR_DEVICE_ATTR(), I'll have to be a bit more flexible since
> > the function _will_ be reused. I'll need something like
> > 	SENSOR_DEVICE_ATTR_{RO,RW,WO}(attr, func, param)
> 
> Chosen at random,
> 
> static SENSOR_DEVICE_ATTR_2(sf2_point4_fan1, S_IRUGO | S_IWUSR,
>                         show_sf2_point, store_sf2_point, 4, 1);
> 
> should become
> 
> static SENSOR_DEVICE_ATTR_2_RW(sf2_point4_fan1, sf2_point, 4, 1); ?
> 
> And the functions should be renamed with show and store at the end?
> 
> > Maybe Greg would be open to something like
> > DEVICE_ATTR_FUNC_{RO,RW,WO}(attr,func)
> > to accommodate the "I want my own function name" crowd ? That would also solve
> > the case where the function is reused for multiple attributes.
> 
> Actually, it was the DEVICE_ATTR_{RO,RW,WO} that wasn't liked.  It doesn't
> show the exact permission numbers.  The fact that not all DEVICE_ATTR uses
> can be changed due to function reuse is awkward, though.  Greg, do you
> have any thoughts about that?

I think the large majority can, and should, be switched to use the
RO,RW,WO variants.  Developers should never be having to specify sysfs
file permissions if at all possible, as they get them wrong too many
times, as we have found out in the past...

> Currently, there are around 1100 calls to DEVICE_ATTR_{RO,RW,WO}.

Those are fine, why do people object to them?

Yes, reusing the function names is not possible, but note, that is a
usually a rare case, the "normal" case for a sysfs attribute can use
these macros just fine.

thanks,

greg k-h

Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time

[Guenter Roeck]

On Thu, Dec 22, 2016 at 04:45:45PM +0100, Julia Lawall wrote:
> 
> 
> On Thu, 22 Dec 2016, Guenter Roeck wrote:
> 
> > On 12/22/2016 04:29 AM, Julia Lawall wrote:
> > >
> > >
> > > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> > >
> > > > Hi Julia,
> > > >
> > > > On Wed, Dec 21, 2016 at 08:39:38PM +0100, Julia Lawall wrote:
> > > > >
> > > > >
> > > > > On Wed, 21 Dec 2016, Guenter Roeck wrote:
> > > > >
> > > > > > Hi Julia,
> > > > > >
> > > > > > On Wed, Dec 21, 2016 at 03:05:37PM +0100, Julia Lawall wrote:
> > > > > > > A solution is below: the semantic patch, an explanation of the
> > > > > > > semantic
> > > > > > > patch, and the results.  I have only tried to compile the results
> > > > > > > (make
> > > > > > > drivers/hwmon/).  Two affected files were not considered for
> > > > > > > compilation:
> > > > > > >
> > > > > > > drivers/hwmon/vexpress-hwmon.o
> > > > > > > drivers/hwmon/jz4740-hwmon.o
> > > >
> > > > I compile tested those two patches. If possible please drop
> > > > vexpress-hwmon.c
> > > > from the patch series; the changes in that file don't add any value.
> > > >
> > > > I compile tested all files, and reviewed the patch. It all looks good.
> > > > Please submit the series.
> > > >
> > > > Again, thanks a lot for your help!
> > >
> > > I have sent the patches.  I adjusted the semantic patch so that the
> > > indentation of function parameters/arguments would only change if the
> > > length of the function name changes.
> > >
> > > Do you think this could be of more general interest in the Linux kernel?
> > > Since the semantic patch works pretty well, I could add it to the
> > > scripts/coccinelle directory?  Previously, however, I got some negative
> > > feedback about this change, because people felt that the new names hid the
> > > actual behavior, so I didn't pursue it.
> > >
> >
> > I do think it would add a lot of value, if for nothing else as an excellent
> > example
> > of what can be done with coccinelle.
> >
> > I actually liked the name changes. I think it is a good idea if the function
> > name
> > reflects the sysfs attribute it serves (isn't that exactly what it does, ie
> > its
> > behavior ?). But, as you have experienced, some people inadvertently did not
> > like
> > it. Given that, I am not sure if it is worth adding it to the kernel source
> > tree.
> > Maybe you could submit it as RFC so it is at least on record.
> >
> > Anyway, for SENSOR_DEVICE_ATTR(), I'll have to be a bit more flexible since
> > the function _will_ be reused. I'll need something like
> > 	SENSOR_DEVICE_ATTR_{RO,RW,WO}(attr, func, param)
> 
> Chosen at random,
> 
> static SENSOR_DEVICE_ATTR_2(sf2_point4_fan1, S_IRUGO | S_IWUSR,
>                         show_sf2_point, store_sf2_point, 4, 1);
> 
> should become
> 
> static SENSOR_DEVICE_ATTR_2_RW(sf2_point4_fan1, sf2_point, 4, 1); ?
> 
> And the functions should be renamed with show and store at the end?
> 
Yes, exactly.  Of course, not all of them use "show_" at the beginning.
get_ and set_ are used as well. Essentially I would want to replace 
	[driver_prefix]{show_,get_,set_}func{_get,_show,_set}
with 'func'.

If you have an idea how to do that, any hints would be welcome.

> > Maybe Greg would be open to something like
> > DEVICE_ATTR_FUNC_{RO,RW,WO}(attr,func)
> > to accommodate the "I want my own function name" crowd ? That would also solve
> > the case where the function is reused for multiple attributes.
> 
> Actually, it was the DEVICE_ATTR_{RO,RW,WO} that wasn't liked.  It doesn't
> show the exact permission numbers.  The fact that not all DEVICE_ATTR uses


It should be obvious that using the {RO,RW,WO} variants is less error prone.
Can anyone seriously argue against that ?

> can be changed due to function reuse is awkward, though.  Greg, do you
> have any thoughts about that?
> 
> Currently, there are around 1100 calls to DEVICE_ATTR_{RO,RW,WO}.
> 

If the problem is that people need to see exact permission numbers instead
of "RO" to understand that an attribute is read only, I think the semantic
patch should really be added to the kernel.

Thanks,
Guenter

Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time

[Guenter Roeck]

Hi Julia,

On Fri, Dec 23, 2016 at 04:54:41PM +0100, Julia Lawall wrote:
[ ... ]
> 
> // -------------------------------------------------------------------------
> @ro@
> declarer name SENSOR_DEVICE_ATTR, SENSOR_DEVICE_ATTR_2;
> identifier x,show;
> @@
> 
> \(SENSOR_DEVICE_ATTR\|SENSOR_DEVICE_ATTR_2\)
>         (x, \(0444\|S_IRUGO\), show, NULL, ...);
> 

my version of spatch (?) does not like this construct.

$ spatch --version
spatch version 1.0.6-00036-gb62df17 compiled with OCaml version 4.02.3
Flags passed to the configure script: --prefix=/usr
Python scripting support: yes
Syntax of regular expresssions: PCRE

Is this something new, or am I doing something wrong ?

I played with it myself, and came up with the (less elegant) version
below. It isn't perfect (it does not handle the function-in-macro case
well), but it should give us a good comparison. Also, I might just get
rid of at least some of those macros; they just obfuscate the code anyway.

Thanks,
Guenter

---
@initialize:python@
@@

import re

// -------------------------------------------------------------------------
// Easy case

@d@
declarer name SENSOR_DEVICE_ATTR,SENSOR_DEVICE_ATTR_2;
identifier x,show,store;
expression p;
@@

(
  SENSOR_DEVICE_ATTR(x,p,show,store,...);
|
  SENSOR_DEVICE_ATTR_2(x,p,show,store,...);
)

@script:python expected@
show << d.show;
store << d.store;
x_show;
x_store;
func;
@@
coccinelle.func = re.sub('show_|get_|_show|_get|_read', '', show)
coccinelle.x_show = re.sub('show_|get_|_show|_get|_read', '', show) + "_show"
coccinelle.x_store = re.sub('show_|get_|_get|_show|_read', '', show) + "_store"

if show == "NULL":
    coccinelle.x_store = re.sub('store_|set_|_set|_store|_write|_reset', '', store) + "_store"
    coccinelle.func = re.sub('store_|set_|_store|_set|_write|_reset', '', store)

@@
declarer name SENSOR_DEVICE_ATTR_RO;
identifier d.x,expected.x_show,expected.func;
expression e;
@@

- SENSOR_DEVICE_ATTR(x, \(0444\|S_IRUGO\), x_show, NULL, e);
+ SENSOR_DEVICE_ATTR_RO(x, func, e);

@@
declarer name SENSOR_DEVICE_ATTR_WO;
identifier d.x,expected.x_store,expected.func;
expression e;
@@

- SENSOR_DEVICE_ATTR(x, \(0200\|S_IWUSR\), NULL, x_store, e);
+ SENSOR_DEVICE_ATTR_WO(x, func, e);

@@
declarer name SENSOR_DEVICE_ATTR_RW;
identifier d.x,expected.x_show,expected.x_store,expected.func;
expression e;
@@

- SENSOR_DEVICE_ATTR(x, \(0644\|S_IRUGO|S_IWUSR\|S_IWUSR|S_IRUGO\), x_show, x_store, e);
+ SENSOR_DEVICE_ATTR_RW(x, func, e);

@@
declarer name SENSOR_DEVICE_ATTR_2_RO;
identifier d.x,expected.x_show,expected.func;
expression e1,e2;
@@

- SENSOR_DEVICE_ATTR_2(x, \(0444\|S_IRUGO\), x_show, NULL, e1, e2);
+ SENSOR_DEVICE_ATTR_2_RO(x, func, e1, e2);

@@
declarer name SENSOR_DEVICE_ATTR_2_WO;
identifier d.x,expected.x_store,expected.func;
expression e1,e2;
@@

- SENSOR_DEVICE_ATTR_2(x, \(0200\|S_IWUSR\), NULL, x_store, e1, e2);
+ SENSOR_DEVICE_ATTR_2_WO(x, func, e1, e2);

@@
declarer name SENSOR_DEVICE_ATTR_2_RW;
identifier d.x,expected.x_show,expected.x_store,expected.func;
expression e1, e2;
@@

- SENSOR_DEVICE_ATTR_2(x, \(0644\|S_IRUGO|S_IWUSR\|S_IWUSR|S_IRUGO\), x_show, x_store, e1, e2);
+ SENSOR_DEVICE_ATTR_2_RW(x, func, e1, e2);

// -------------------------------------------------------------------------
// Other calls

@o@
declarer name SENSOR_DEVICE_ATTR,SENSOR_DEVICE_ATTR_2;
identifier d.x,show,store;
expression list es;
@@

(
SENSOR_DEVICE_ATTR(x,\(0444\|S_IRUGO\|0200\|S_IWUSR\|0644\|S_IRUGO|S_IWUSR\|S_IWUSR|S_IRUGO\),show,store,es);
|
SENSOR_DEVICE_ATTR_2(x,\(0444\|S_IRUGO\|0200\|S_IWUSR\|0644\|S_IRUGO|S_IWUSR\|S_IWUSR|S_IRUGO\),show,store,es);
)

// rename functions

@show1@
identifier o.show,expected.x_show;
parameter list ps;
@@

static
- show(ps)
+ x_show(ps)
  { ... }

@depends on show1@
identifier o.show,expected.x_show;
expression list es;
@@
- show(es)
+ x_show(es)

@depends on show1@
identifier o.show,expected.x_show;
@@
- show
+ x_show

@store1@
identifier o.store,expected.x_store;
parameter list ps;
@@

static
- store(ps)
+ x_store(ps)
  { ... }

@depends on store1@
identifier o.store,expected.x_store;
expression list es;
@@
- store(es)
+ x_store(es)

@depends on store1@
identifier o.store,expected.x_store;
@@
- store
+ x_store

// try again

@@
declarer name SENSOR_DEVICE_ATTR_RO;
identifier d.x,expected.x_show,expected.func;
expression e;
@@

- SENSOR_DEVICE_ATTR(x, \(0444\|S_IRUGO\), x_show, NULL, e);
+ SENSOR_DEVICE_ATTR_RO(x, func, e);

@@
declarer name SENSOR_DEVICE_ATTR_WO;
identifier d.x,expected.x_store,expected.func;
expression e;
@@

- SENSOR_DEVICE_ATTR(x, \(0200\|S_IWUSR\), NULL, x_store, e);
+ SENSOR_DEVICE_ATTR_WO(x, func, e);

@@
declarer name SENSOR_DEVICE_ATTR_RW;
identifier d.x,expected.x_show,expected.x_store,expected.func;
expression e;
@@

- SENSOR_DEVICE_ATTR(x, \(0644\|S_IRUGO|S_IWUSR\|S_IWUSR|S_IRUGO\), x_show, x_store, e);
+ SENSOR_DEVICE_ATTR_RW(x, func, e);

@@
declarer name SENSOR_DEVICE_ATTR_2_RO;
identifier d.x,expected.x_show,expected.func;
expression e1,e2;
@@

- SENSOR_DEVICE_ATTR_2(x, \(0444\|S_IRUGO\), x_show, NULL, e1, e2);
+ SENSOR_DEVICE_ATTR_2_RO(x, func, e1, e2);

@@
declarer name SENSOR_DEVICE_ATTR_2_WO;
identifier d.x,expected.x_store,expected.func;
expression e1, e2;
@@

- SENSOR_DEVICE_ATTR_2(x, \(0200\|S_IWUSR\), NULL, x_store, e1, e2);
+ SENSOR_DEVICE_ATTR_2_WO(x, func, e1, e2);

@@
declarer name SENSOR_DEVICE_ATTR_2_RW;
identifier d.x,expected.x_show,expected.x_store,expected.func;
expression e1, e2;
@@

- SENSOR_DEVICE_ATTR_2(x, \(0644\|S_IRUGO|S_IWUSR\|S_IWUSR|S_IRUGO\), x_show, x_store, e1, e2);
+ SENSOR_DEVICE_ATTR_2_RW(x, func, e1, e2);

Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time

[Julia Lawall]

On Fri, 23 Dec 2016, Guenter Roeck wrote:

> Hi Julia,
>
> On Fri, Dec 23, 2016 at 04:54:41PM +0100, Julia Lawall wrote:
> [ ... ]
> >
> > // -------------------------------------------------------------------------
> > @ro@
> > declarer name SENSOR_DEVICE_ATTR, SENSOR_DEVICE_ATTR_2;
> > identifier x,show;
> > @@
> >
> > \(SENSOR_DEVICE_ATTR\|SENSOR_DEVICE_ATTR_2\)
> >         (x, \(0444\|S_IRUGO\), show, NULL, ...);
> >
>
> my version of spatch (?) does not like this construct.
>
> $ spatch --version
> spatch version 1.0.6-00036-gb62df17 compiled with OCaml version 4.02.3
> Flags passed to the configure script: --prefix=/usr
> Python scripting support: yes
> Syntax of regular expresssions: PCRE
>
> Is this something new, or am I doing something wrong ?

Something new, sorry.  I think it should be available in the github
version.  If not, the solution is just to split each case like this up
into two rules.

> I played with it myself, and came up with the (less elegant) version
> below. It isn't perfect (it does not handle the function-in-macro case
> well), but it should give us a good comparison. Also, I might just get
> rid of at least some of those macros; they just obfuscate the code anyway.
>
> Thanks,
> Guenter
>
> ---
> @initialize:python@
> @@
>
> import re
>
> // -------------------------------------------------------------------------
> // Easy case
>
> @d@
> declarer name SENSOR_DEVICE_ATTR,SENSOR_DEVICE_ATTR_2;
> identifier x,show,store;
> expression p;
> @@
>
> (
>   SENSOR_DEVICE_ATTR(x,p,show,store,...);
> |
>   SENSOR_DEVICE_ATTR_2(x,p,show,store,...);
> )
>
> @script:python expected@
> show << d.show;
> store << d.store;
> x_show;
> x_store;
> func;
> @@
> coccinelle.func = re.sub('show_|get_|_show|_get|_read', '', show)
> coccinelle.x_show = re.sub('show_|get_|_show|_get|_read', '', show) + "_show"
> coccinelle.x_store = re.sub('show_|get_|_get|_show|_read', '', show) +
"_store"
>
> if show == "NULL":
>     coccinelle.x_store = re.sub('store_|set_|_set|_store|_write|_reset', '', store) + "_store"
>     coccinelle.func = re.sub('store_|set_|_store|_set|_write|_reset', '', store)

OK, so you solve the incompatability between show and store by jut taking
the show version.  There could be some danger that another call will only
have the same store function and will rename it in a different way.  That
is:

SENSOR_DEVICE_ATTR(x, opt, get_one, set_two, 0)

will rename set_two to one_store, and

SENSOR_DEVICE_ATTR(x, opt, NULL, set_two, 0)

will rename set_two to two_store.

Otherwise, it seems fine.  It is better than my version in that it chooses
the new names all at once.  My version chose the new names for eg the WO
and RW cases after the RO case, so it had to protect against rechanging
names that it had already changed.  I also protect against renaming a name
being used in a DEVICE_ATTR call.  I don't know if that can happen.

You don't actually havce to repeat the declarer name declaration in every
rule.  Once you have specified that once, it is valid forever.  New
versions of Coccinelle allow the redeclaration, butold versions will
break.

julia


> @@
> declarer name SENSOR_DEVICE_ATTR_RO;
> identifier d.x,expected.x_show,expected.func;
> expression e;
> @@
>
> - SENSOR_DEVICE_ATTR(x, \(0444\|S_IRUGO\), x_show, NULL, e);
> + SENSOR_DEVICE_ATTR_RO(x, func, e);
>
> @@
> declarer name SENSOR_DEVICE_ATTR_WO;
> identifier d.x,expected.x_store,expected.func;
> expression e;
> @@
>
> - SENSOR_DEVICE_ATTR(x, \(0200\|S_IWUSR\), NULL, x_store, e);
> + SENSOR_DEVICE_ATTR_WO(x, func, e);
>
> @@
> declarer name SENSOR_DEVICE_ATTR_RW;
> identifier d.x,expected.x_show,expected.x_store,expected.func;
> expression e;
> @@
>
> - SENSOR_DEVICE_ATTR(x, \(0644\|S_IRUGO|S_IWUSR\|S_IWUSR|S_IRUGO\), x_show, x_store, e);
> + SENSOR_DEVICE_ATTR_RW(x, func, e);
>
> @@
> declarer name SENSOR_DEVICE_ATTR_2_RO;
> identifier d.x,expected.x_show,expected.func;
> expression e1,e2;
> @@
>
> - SENSOR_DEVICE_ATTR_2(x, \(0444\|S_IRUGO\), x_show, NULL, e1, e2);
> + SENSOR_DEVICE_ATTR_2_RO(x, func, e1, e2);
>
> @@
> declarer name SENSOR_DEVICE_ATTR_2_WO;
> identifier d.x,expected.x_store,expected.func;
> expression e1,e2;
> @@
>
> - SENSOR_DEVICE_ATTR_2(x, \(0200\|S_IWUSR\), NULL, x_store, e1, e2);
> + SENSOR_DEVICE_ATTR_2_WO(x, func, e1, e2);
>
> @@
> declarer name SENSOR_DEVICE_ATTR_2_RW;
> identifier d.x,expected.x_show,expected.x_store,expected.func;
> expression e1, e2;
> @@
>
> - SENSOR_DEVICE_ATTR_2(x, \(0644\|S_IRUGO|S_IWUSR\|S_IWUSR|S_IRUGO\), x_show, x_store, e1, e2);
> + SENSOR_DEVICE_ATTR_2_RW(x, func, e1, e2);
>
> // -------------------------------------------------------------------------
> // Other calls
>
> @o@
> declarer name SENSOR_DEVICE_ATTR,SENSOR_DEVICE_ATTR_2;
> identifier d.x,show,store;
> expression list es;
> @@
>
> (
> SENSOR_DEVICE_ATTR(x,\(0444\|S_IRUGO\|0200\|S_IWUSR\|0644\|S_IRUGO|S_IWUSR\|S_IWUSR|S_IRUGO\),show,store,es);
> |
> SENSOR_DEVICE_ATTR_2(x,\(0444\|S_IRUGO\|0200\|S_IWUSR\|0644\|S_IRUGO|S_IWUSR\|S_IWUSR|S_IRUGO\),show,store,es);
> )
>
> // rename functions
>
> @show1@
> identifier o.show,expected.x_show;
> parameter list ps;
> @@
>
> static
> - show(ps)
> + x_show(ps)
>   { ... }
>
> @depends on show1@
> identifier o.show,expected.x_show;
> expression list es;
> @@
> - show(es)
> + x_show(es)
>
> @depends on show1@
> identifier o.show,expected.x_show;
> @@
> - show
> + x_show
>
> @store1@
> identifier o.store,expected.x_store;
> parameter list ps;
> @@
>
> static
> - store(ps)
> + x_store(ps)
>   { ... }
>
> @depends on store1@
> identifier o.store,expected.x_store;
> expression list es;
> @@
> - store(es)
> + x_store(es)
>
> @depends on store1@
> identifier o.store,expected.x_store;
> @@
> - store
> + x_store
>
> // try again
>
> @@
> declarer name SENSOR_DEVICE_ATTR_RO;
> identifier d.x,expected.x_show,expected.func;
> expression e;
> @@
>
> - SENSOR_DEVICE_ATTR(x, \(0444\|S_IRUGO\), x_show, NULL, e);
> + SENSOR_DEVICE_ATTR_RO(x, func, e);
>
> @@
> declarer name SENSOR_DEVICE_ATTR_WO;
> identifier d.x,expected.x_store,expected.func;
> expression e;
> @@
>
> - SENSOR_DEVICE_ATTR(x, \(0200\|S_IWUSR\), NULL, x_store, e);
> + SENSOR_DEVICE_ATTR_WO(x, func, e);
>
> @@
> declarer name SENSOR_DEVICE_ATTR_RW;
> identifier d.x,expected.x_show,expected.x_store,expected.func;
> expression e;
> @@
>
> - SENSOR_DEVICE_ATTR(x, \(0644\|S_IRUGO|S_IWUSR\|S_IWUSR|S_IRUGO\), x_show, x_store, e);
> + SENSOR_DEVICE_ATTR_RW(x, func, e);
>
> @@
> declarer name SENSOR_DEVICE_ATTR_2_RO;
> identifier d.x,expected.x_show,expected.func;
> expression e1,e2;
> @@
>
> - SENSOR_DEVICE_ATTR_2(x, \(0444\|S_IRUGO\), x_show, NULL, e1, e2);
> + SENSOR_DEVICE_ATTR_2_RO(x, func, e1, e2);
>
> @@
> declarer name SENSOR_DEVICE_ATTR_2_WO;
> identifier d.x,expected.x_store,expected.func;
> expression e1, e2;
> @@
>
> - SENSOR_DEVICE_ATTR_2(x, \(0200\|S_IWUSR\), NULL, x_store, e1, e2);
> + SENSOR_DEVICE_ATTR_2_WO(x, func, e1, e2);
>
> @@
> declarer name SENSOR_DEVICE_ATTR_2_RW;
> identifier d.x,expected.x_show,expected.x_store,expected.func;
> expression e1, e2;
> @@
>
> - SENSOR_DEVICE_ATTR_2(x, \(0644\|S_IRUGO|S_IWUSR\|S_IWUSR|S_IRUGO\), x_show, x_store, e1, e2);
> + SENSOR_DEVICE_ATTR_2_RW(x, func, e1, e2);
>