Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Sun, 27 Sep 2020, Joe Perches wrote: > On Sun, 2020-09-27 at 19:08 +0200, Julia Lawall wrote: > > I end up with 208 patches. I'm not sure that sending them all at once > > would be a good idea... > > Last I looked the diffstat for comma -> semicolon was: > > 234 files changed, 509 insertions(+), 509 deletions(-) > > So it would be nearly 1 patch per individual file, I have 282 files. > > Greg KH does send hundreds of patches for -stable at a time. > > So, maybe or maybe not send them all at once. > Maybe send it in batches of 25 or so. > > There's no single right way to do this. > > Maybe put up a git tree somewhere and let the > kernel-robot test compilation. I compiled all but about 15 and checked those 15 an extra time. I'll try the small batch approach to get started. thanks, julia > (A nicety might be for the kernel-robot to have some > option to test pre and post compilation object code > differences with an optional report) > > When I automated 491 patches for /* fallthrough */ to > fallthrough;, the robot caught a couple problems which > was great. > > https://repo.or.cz/linux-2.6/trivial-mods.git/shortlog/refs/heads/20200310_fallthrough_2 > > I only posted the first ~30 patches though with > about 50% acceptance. Gustavo Silva picked up the > effort and did a great job. Eventually, a single > treewide patch was posted and accepted by Linus for > this though after dozens of individual patches went > through various maintainer trees: > > $ git log --shortstat -1 df561f6688fe > commit df561f6688fef775baa341a0f5d960becd248b11 > Author: Gustavo A. R. Silva > Date: Sun Aug 23 17:36:59 2020 -0500 > > treewide: Use fallthrough pseudo-keyword > > Replace the existing /* fall through */ comments and its variants with > the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary > fall-through markings when it is the case. > > [1] > https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=> > > Signed-off-by: Gustavo A. R. Silva > > 1148 files changed, 2667 insertions(+), 2737 deletions(-) > > > ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Sun, 2020-09-27 at 19:08 +0200, Julia Lawall wrote: > I end up with 208 patches. I'm not sure that sending them all at once > would be a good idea... Last I looked the diffstat for comma -> semicolon was: 234 files changed, 509 insertions(+), 509 deletions(-) So it would be nearly 1 patch per individual file, Greg KH does send hundreds of patches for -stable at a time. So, maybe or maybe not send them all at once. Maybe send it in batches of 25 or so. There's no single right way to do this. Maybe put up a git tree somewhere and let the kernel-robot test compilation. (A nicety might be for the kernel-robot to have some option to test pre and post compilation object code differences with an optional report) When I automated 491 patches for /* fallthrough */ to fallthrough;, the robot caught a couple problems which was great. https://repo.or.cz/linux-2.6/trivial-mods.git/shortlog/refs/heads/20200310_fallthrough_2 I only posted the first ~30 patches though with about 50% acceptance. Gustavo Silva picked up the effort and did a great job. Eventually, a single treewide patch was posted and accepted by Linus for this though after dozens of individual patches went through various maintainer trees: $ git log --shortstat -1 df561f6688fe commit df561f6688fef775baa341a0f5d960becd248b11 Author: Gustavo A. R. Silva Date: Sun Aug 23 17:36:59 2020 -0500 treewide: Use fallthrough pseudo-keyword Replace the existing /* fall through */ comments and its variants with the new pseudo-keyword macro fallthrough[1]. Also, remove unnecessary fall-through markings when it is the case. [1] https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=> Signed-off-by: Gustavo A. R. Silva 1148 files changed, 2667 insertions(+), 2737 deletions(-) ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
I end up with 208 patches. I'm not sure that sending them all at once would be a good idea... julia ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Fri, 25 Sep 2020 10:26:27 -0700, Joe Perches said: > And the generic individual maintainer apply rate for > each specific patch is always less than 50%. > > For instance the patches that converted the comma uses > in if/do/while statements to use braces and semicolons > from a month ago: > 29 patches, 13 applied. To be fair, it's *always* been hard to get pure style patches applied, because they usually hit one of two types of code, with different results: Some of them hit code that's been stable for a long time - and those patches don't get applied because of the (admittedly small) risk that a "style" patch may actually break something - yes, that *does* happen often enough to worry a risk-adverse subtree maintainer. Some of them hit code that's actively being worked on - and those patches don't get applied because they can cause merge conflicts. This is a hard problem to fix, because it's difficult to say that either of those viewpoints is *totally* wrong. At best, you can make the case that some maintainers are a tad over-zealous on their attitude. And since its *hard* to find good maintainers, it's not possible to fix the problem by just putting somebody else in charge of a subtree. It's theoretically possible to bypass a problematic maintainer by sending the patch to the person one level up, or directly to Linus - but although that usually works if you have an urgent patch and the maintainer is on vacation or stubborn or whatever, that's got essentially zero chance of succeeding for a mere style patch. Unfortunately, although I understand the problem, I don't have a solution. It's easy to tactfully say "this code is wrong, and here is the fix". It's a lot harder to find a tactful way to say "This person is wrong and should do it this way", because code doesn't fight back when you offer constructive criticism pgpw2dLOfrk_r.pgp Description: PGP signature ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Fri, 2020-09-25 at 19:06 +0200, Julia Lawall wrote: > On Thu, 24 Sep 2020, Joe Perches wrote: > > On Thu, 2020-09-24 at 23:53 +0200, Thomas Gleixner wrote: > > > On Thu, Sep 24 2020 at 13:33, Joe Perches wrote: > > > > On Thu, 2020-09-24 at 22:19 +0200, Thomas Gleixner wrote: > > > > > 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? > > > > > > > > Treewide? > > > > > > > > Somebody no doubt would complain, but there > > > > _really should_ be some mechanism for these > > > > trivial and correct treewide changes... > > > > > > There are lots of mechanisms: > > > > I've tried them all. > > > > None of them work particularly well, > > especially the individual patch route. > > > > > - Andrew picks such changes up > > > > Generally not treewide. > > > > > - With a few competent eyeballs on it (reviewers) this can go thorugh > > >the trivial tree as well. It's more than obvious after all. > > > > Jiri is almost non-existent when it comes to > > trivial treewide patches. > > > > > - Send the script to Linus with a proper change log attached and ask > > >him to run it. > > > > Linus has concerns about backports and what he > > deems trivialities. Generally overblown IMO. > > > > > - In the worst case if nobody feels responsible, I'll take care. > > > > If Julia doesn't send a new patch in the next few > > days, I will do the apply, fixup and resend of hers. > > > > So, you're on-deck, nearly up... > > > > > All of the above is better than trying to get the attention of a > > > gazillion of maintainters. > > > > True. > > > > And all of the treewide changes depend on some > > generic acceptance of value in the type of change. > > > > Some believe that comma->semicolon conversions > > aren't useful as there isn't a logical change and > > the compiler output wouldn't be different. > > I have a script that will cut up the patches and send them to the > appropriate maintainers, so I have no problem with that route. I have a script that does that too. The complaint I get about its use is "OMG: My specific commit header style isn't followed" And the generic individual maintainer apply rate for each specific patch is always less than 50%. For instance the patches that converted the comma uses in if/do/while statements to use braces and semicolons from a month ago: https://lore.kernel.org/lkml/cover.1598331148.git@perches.com/ 29 patches, 13 applied. Best of luck. ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Thu, 24 Sep 2020, Joe Perches wrote: > On Thu, 2020-09-24 at 23:53 +0200, Thomas Gleixner wrote: > > On Thu, Sep 24 2020 at 13:33, Joe Perches wrote: > > > On Thu, 2020-09-24 at 22:19 +0200, Thomas Gleixner wrote: > > > > 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? > > > > > > Treewide? > > > > > > Somebody no doubt would complain, but there > > > _really should_ be some mechanism for these > > > trivial and correct treewide changes... > > > > There are lots of mechanisms: > > I've tried them all. > > None of them work particularly well, > especially the individual patch route. > > > - Andrew picks such changes up > > Generally not treewide. > > > - With a few competent eyeballs on it (reviewers) this can go thorugh > >the trivial tree as well. It's more than obvious after all. > > Jiri is almost non-existent when it comes to > trivial treewide patches. > > > - Send the script to Linus with a proper change log attached and ask > >him to run it. > > Linus has concerns about backports and what he > deems trivialities. Generally overblown IMO. > > > - In the worst case if nobody feels responsible, I'll take care. > > If Julia doesn't send a new patch in the next few > days, I will do the apply, fixup and resend of hers. > > So, you're on-deck, nearly up... > > > All of the above is better than trying to get the attention of a > > gazillion of maintainters. > > True. > > And all of the treewide changes depend on some > generic acceptance of value in the type of change. > > Some believe that comma->semicolon conversions > aren't useful as there isn't a logical change and > the compiler output wouldn't be different. I have a script that will cut up the patches and send them to the appropriate maintainers, so I have no problem with that route. julia ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Thu, 2020-09-24 at 23:53 +0200, Thomas Gleixner wrote: > On Thu, Sep 24 2020 at 13:33, Joe Perches wrote: > > On Thu, 2020-09-24 at 22:19 +0200, Thomas Gleixner wrote: > > > 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? > > > > Treewide? > > > > Somebody no doubt would complain, but there > > _really should_ be some mechanism for these > > trivial and correct treewide changes... > > There are lots of mechanisms: I've tried them all. None of them work particularly well, especially the individual patch route. > - Andrew picks such changes up Generally not treewide. > - With a few competent eyeballs on it (reviewers) this can go thorugh >the trivial tree as well. It's more than obvious after all. Jiri is almost non-existent when it comes to trivial treewide patches. > - Send the script to Linus with a proper change log attached and ask >him to run it. Linus has concerns about backports and what he deems trivialities. Generally overblown IMO. > - In the worst case if nobody feels responsible, I'll take care. If Julia doesn't send a new patch in the next few days, I will do the apply, fixup and resend of hers. So, you're on-deck, nearly up... > All of the above is better than trying to get the attention of a > gazillion of maintainters. True. And all of the treewide changes depend on some generic acceptance of value in the type of change. Some believe that comma->semicolon conversions aren't useful as there isn't a logical change and the compiler output wouldn't be different. Anyway, cheers, Joe ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Thu, Sep 24 2020 at 13:33, Joe Perches wrote: > On Thu, 2020-09-24 at 22:19 +0200, Thomas Gleixner wrote: >> 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? > > Treewide? > > Somebody no doubt would complain, but there > _really should_ be some mechanism for these > trivial and correct treewide changes... There are lots of mechanisms: - Andrew picks such changes up - With a few competent eyeballs on it (reviewers) this can go thorugh the trivial tree as well. It's more than obvious after all. - Send the script to Linus with a proper change log attached and ask him to run it. - In the worst case if nobody feels responsible, I'll take care. All of the above is better than trying to get the attention of a gazillion of maintainters. Thanks, tglx ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Thu, 2020-09-24 at 22:19 +0200, Thomas Gleixner wrote: > 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? Treewide? Somebody no doubt would complain, but there _really should_ be some mechanism for these trivial and correct treewide changes... ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Thu, 24 Sep 2020, Thomas Gleixner wrote: > 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. I will try to get to it soon. Thanks for checking all the cases. julia > > Thanks, > > tglx > ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
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 ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
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 {}.
pgp_fOcGf0sP7.pgp
Description: PGP signature
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
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___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
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.
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
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 = _reset_ops,
> + rc->rst.ops = _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...
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
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.
Great, thanks.
> > 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?
Ideally for linux kernel style, yes.
> 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.
I would probably just do those by hand, I believe
there are only a few dozen and they are easily found
using the original script.
> 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.
(150kb patch removed)
There sure are a bunch of these...
This output patch is very difficult to read as it's unordered.
Perhaps it'd be simpler using --in-place and git diff --stat -p
Thanks again, Joe
btw: the ordered diffstat for Julia's removed patch is:
$ git diff --stat
arch/alpha/kernel/process.c| 2 +-
arch/arm/mach-davinci/pm.c | 2 +-
arch/arm/mach-ixp4xx/ixdp425-setup.c | 2 +-
arch/arm/mach-pxa/eseries.c| 6 +--
arch/arm/mach-pxa/palm27x.c| 4 +-
arch/arm/mach-pxa/z2.c | 2 +-
arch/arm/vfp/vfp.h | 2 +-
arch/ia64/kernel/setup.c | 2 +-
arch/m68k/lib/muldi3.c | 2 +-
arch/mips/bcm63xx/dev-spi.c| 4 +-
arch/mips/kernel/cevt-txx9.c | 2 +-
arch/mips/kernel/vpe-cmp.c | 4 +-
arch/mips/kernel/vpe-mt.c | 4 +-
arch/mips/pci/pci-ar2315.c | 6 +--
arch/openrisc/kernel/time.c| 8 ++--
arch/powerpc/kexec/core.c | 2 +-
arch/powerpc/lib/feature-fixups.c | 8 ++--
arch/sparc/kernel/pci_sun4v.c | 2 +-
arch/x86/kernel/cpu/mtrr/cleanup.c | 4 +-
.../platform/intel-mid/device_libs/platform_bt.c | 4 +-
block/bsg-lib.c| 2 +-
drivers/base/regmap/regmap-debugfs.c | 2 +-
drivers/bcma/driver_pci_host.c | 4 +-
drivers/char/agp/amd-k7-agp.c | 2 +-
drivers/char/agp/nvidia-agp.c | 2 +-
drivers/char/agp/sworks-agp.c | 2 +-
drivers/char/hw_random/iproc-rng200.c | 8 ++--
drivers/char/hw_random/mxc-rnga.c | 6 +--
drivers/char/hw_random/stm32-rng.c | 8 ++--
drivers/char/ipmi/bt-bmc.c | 6 +--
drivers/clk/meson/meson-aoclk.c| 2 +-
drivers/clk/mvebu/ap-cpu-clk.c
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Thu, 2020-08-20 at 10:33 +0200, Julia Lawall wrote:
> On Wed, 19 Aug 2020, Joe Perches wrote:
> > On Wed, 2020-08-19 at 14:22 -0700, Joe Perches wrote:
> > > There are commas used as statement terminations that should typically
> > > have used semicolons instead. Only direct assignments or use of a single
> > > function or value on a single line are detected by this test.
> > >
> > > e.g.:
> > > foo = bar(),/* typical use is semicolon not comma */
> > > bar = baz();
> > >
> > > Add an imperfect test to detect these comma uses.
> > >
> > > No false positives were found in testing, but many types of false
> > > negatives
> > > are possible.
> > >
> > > e.g.:
> > > foo = bar() + 1,/* comma use, but not direct assignment */
> > > bar = baz();
> >
> > Hi.
> >
> > I recently added a test for this condition to linux's checkpatch.
> >
> > A similar coccinelle script might be:
> >
> > $ cat comma.cocci
> > @@
> > expression e1;
> > expression e2;
> > @@
> >
> > e1
> > - ,
> > + ;
> > e2;
> > $
> >
> > This works reasonably well but it has several false positives
> > for declarations like:
> >
> > $ spatch --sp-file comma.cocci mm/huge_memory.c
> > diff -u -p a/huge_memory.c b/huge_memory.c
> > --- a/huge_memory.c
> > +++ b/huge_memory.c
> > @@ -2778,7 +2778,7 @@ static unsigned long deferred_split_scan
> > struct pglist_data *pgdata = NODE_DATA(sc->nid);
> > struct deferred_split *ds_queue = >deferred_split_queue;
> > unsigned long flags;
> > - LIST_HEAD(list), *pos, *next;
> > + LIST_HEAD(list), *pos; *next;
> > struct page *page;
> > int split = 0;
> > $
> >
> > Any script improvement suggestions?
>
> 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,
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;
}
There are a few dozen uses of this style in linux kernel.
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Wed, 19 Aug 2020, Joe Perches wrote: > On Wed, 2020-08-19 at 14:22 -0700, Joe Perches wrote: > > There are commas used as statement terminations that should typically > > have used semicolons instead. Only direct assignments or use of a single > > function or value on a single line are detected by this test. > > > > e.g.: > > foo = bar(),/* typical use is semicolon not comma */ > > bar = baz(); > > > > Add an imperfect test to detect these comma uses. > > > > No false positives were found in testing, but many types of false negatives > > are possible. > > > > e.g.: > > foo = bar() + 1,/* comma use, but not direct assignment */ > > bar = baz(); > > Hi. > > I recently added a test for this condition to linux's checkpatch. > > A similar coccinelle script might be: > > $ cat comma.cocci > @@ > expression e1; > expression e2; > @@ > > e1 > - , > + ; > e2; > $ > > This works reasonably well but it has several false positives > for declarations like: > > $ spatch --sp-file comma.cocci mm/huge_memory.c > diff -u -p a/huge_memory.c b/huge_memory.c > --- a/huge_memory.c > +++ b/huge_memory.c > @@ -2778,7 +2778,7 @@ static unsigned long deferred_split_scan > struct pglist_data *pgdata = NODE_DATA(sc->nid); > struct deferred_split *ds_queue = >deferred_split_queue; > unsigned long flags; > - LIST_HEAD(list), *pos, *next; > + LIST_HEAD(list), *pos; *next; > struct page *page; > int split = 0; > $ > > Any script improvement suggestions? 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. thanks, julia ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
On Wed, 2020-08-19 at 14:22 -0700, Joe Perches wrote: > There are commas used as statement terminations that should typically > have used semicolons instead. Only direct assignments or use of a single > function or value on a single line are detected by this test. > > e.g.: > foo = bar(),/* typical use is semicolon not comma */ > bar = baz(); > > Add an imperfect test to detect these comma uses. > > No false positives were found in testing, but many types of false negatives > are possible. > > e.g.: > foo = bar() + 1,/* comma use, but not direct assignment */ > bar = baz(); Hi. I recently added a test for this condition to linux's checkpatch. A similar coccinelle script might be: $ cat comma.cocci @@ expression e1; expression e2; @@ e1 - , + ; e2; $ This works reasonably well but it has several false positives for declarations like: $ spatch --sp-file comma.cocci mm/huge_memory.c diff -u -p a/huge_memory.c b/huge_memory.c --- a/huge_memory.c +++ b/huge_memory.c @@ -2778,7 +2778,7 @@ static unsigned long deferred_split_scan struct pglist_data *pgdata = NODE_DATA(sc->nid); struct deferred_split *ds_queue = >deferred_split_queue; unsigned long flags; - LIST_HEAD(list), *pos, *next; + LIST_HEAD(list), *pos; *next; struct page *page; int split = 0; $ Any script improvement suggestions? ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
