Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
(forwarding on to kernel-janitors/mentees and kernelnewbies) Just fyi for anyone that cares: A janitorial task for someone might be to use Julia's coccinelle script below to convert the existing instances of commas that separate statements into semicolons. https://lore.kernel.org/lkml/alpine.DEB.2.22.394.2008201856110.2524@hadrien/ On Thu, 2020-08-20 at 19:03 +0200, Julia Lawall wrote:
I have a bunch of variations of this that are more complicated than I would have expected. One shorter variant that I have is:
@@ expression e1,e2; statement S; @@
S e1 -, +; (<+... e2 ...+>);
This will miss cases where the first statement is the comma thing. But I think it is possible to improve this now. I will check.
Hi Julia.
Right, thanks, this adds a dependency on a statement before the expression. Any stragglers would be easily found using slightly different form. There are not very many of these in linux kernel.
Another nicety would be to allow the s/,/;/ conversion to find both b and c in this sequence: a = 1; b = 2, c = 3, d = 4; without running the script multiple times. There are many dozen uses of this style in linux kernel.
I tried variants of adding a comma after the e2 expression, but cocci seems to have parsing problems with:
@@ expression e1; expression e2; @@ e1 - , + ; e2,
This doesn't work because it's not a valid expression.
The problem is solved by doing:
e1 - , + ; e2 ... when any
But that doesn't work in the current version of Coccinelle. I have fixed the problem, though and it will work shortly.
I do appreciate that coccinelle adds braces for multiple expression comma use after an if.
i.e.: if (foo) a = 1, b = 2; becomes if (foo) { a = 1; b = 2; }
I wasn't sure what was wanted for such things. Should b = 2 now be on a separate line?
I took the strategy of avoiding the problem and leaving those cases as is. That also solves the LIST_HEAD problem. But if it is wanted to change these commas under ifs, then that is probably possible too.
My current complete solution is as follows. The first rule avoids changing commas in macros, where thebody of the macro is just an expression. The second rule uses position variables to ensure that the two expression are on different lines.
@r@ expression e1,e2; statement S; position p; @@
e1 ,@S@p e2;
@@ expression e1,e2; position p1; position p2 : script:ocaml(p1) { not((List.hd p1).line_end = (List.hd p2).line) }; statement S; position r.p; @@
e1@p1 -,@S@p +; e2@p2 ... when any
The generated patch is below.
julia
diff -u -p a/drivers/reset/hisilicon/reset-hi3660.c b/drivers/reset/hisilicon/reset-hi3660.c --- a/drivers/reset/hisilicon/reset-hi3660.c +++ b/drivers/reset/hisilicon/reset-hi3660.c @@ -89,7 +89,7 @@ static int hi3660_reset_probe(struct pla return PTR_ERR(rc->map); }
- rc->rst.ops = &hi3660_reset_ops, + rc->rst.ops = &hi3660_reset_ops; rc->rst.of_node = np; rc->rst.of_reset_n_cells = 2; rc->rst.of_xlate = hi3660_reset_xlate;
The rest of the changes are in the link above...
On Fri, 21 Aug 2020 18:08:08 -0700, Joe Perches said:
(forwarding on to kernel-janitors/mentees and kernelnewbies)
Just fyi for anyone that cares:
A janitorial task for someone might be to use Julia's coccinelle script below to convert the existing instances of commas that separate statements into semicolons.
Note that you need to *really* check for possible changes in semantics. It's *usually* OK to do that, but sometimes it's not... for (i=0; i++, last++; !last) { changing that comma to a ; will break the compile. In other cases, it can introduce subtle bugs.
I do appreciate that coccinelle adds braces for multiple expression comma use after an if.
i.e.: if (foo) a = 1, b = 2; becomes if (foo) { a = 1; b = 2; }
Yeah. Like there, if you forget to add the {}.
On Fri, 2020-08-21 at 23:35 -0400, Valdis Klētnieks wrote:
On Fri, 21 Aug 2020 18:08:08 -0700, Joe Perches said:
(forwarding on to kernel-janitors/mentees and kernelnewbies)
Just fyi for anyone that cares:
A janitorial task for someone might be to use Julia's coccinelle script below to convert the existing instances of commas that separate statements into semicolons.
Note that you need to *really* check for possible changes in semantics. It's *usually* OK to do that, but sometimes it's not...
for (i=0; i++, last++; !last) {
changing that comma to a ; will break the compile. In other cases, it can introduce subtle bugs.
True enough for a general statement, though the coccinelle script Julia provided does not change a single instance of for loop expressions with commas. As far as I can tell, no logic defect is introduced by the script at all.
On Fri, 21 Aug 2020, Joe Perches wrote:
On Fri, 2020-08-21 at 23:35 -0400, Valdis Klētnieks wrote:
On Fri, 21 Aug 2020 18:08:08 -0700, Joe Perches said:
(forwarding on to kernel-janitors/mentees and kernelnewbies)
Just fyi for anyone that cares:
A janitorial task for someone might be to use Julia's coccinelle script below to convert the existing instances of commas that separate statements into semicolons.
Note that you need to *really* check for possible changes in semantics. It's *usually* OK to do that, but sometimes it's not...
for (i=0; i++, last++; !last) {
changing that comma to a ; will break the compile. In other cases, it can introduce subtle bugs.
True enough for a general statement, though the coccinelle script Julia provided does not change a single instance of for loop expressions with commas.
As far as I can tell, no logic defect is introduced by the script at all.
The script has a rule to ensure that what is changed is part of a top level statement that has the form e1, e2;. I put that in to avoid transforming cases where the comma is the body of a macro, but it protects against for loop headers as well. julia
On Sat, Aug 22 2020 at 09:07, Julia Lawall wrote:
On Fri, 21 Aug 2020, Joe Perches wrote:
True enough for a general statement, though the coccinelle script Julia provided does not change a single instance of for loop expressions with commas.
As far as I can tell, no logic defect is introduced by the script at all.
The script has a rule to ensure that what is changed is part of a top level statement that has the form e1, e2;. I put that in to avoid transforming cases where the comma is the body of a macro, but it protects against for loop headers as well.
Right. I went through the lot and did not find something dodgy. Except for two hunks this still applies. Can someone please send a proper patch with changelog/SOB etc. for this? And of course that script really wants to be part of the kernel cocci checks to catch further instances. Thanks, tglx
participants (4)
-
Joe Perches -
Julia Lawall -
Thomas Gleixner -
Valdis Klētnieks