Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time
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); > | >
Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time
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); > | >
Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time
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
Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time
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
Re: Converting DEVICE_ATTR to DEVICE_ATTR_{RO,RW,WO} and changing function names at the same time
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
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
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
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
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
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
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
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
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
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