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
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
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.joe@perches.com/ 29 patches, 13 applied. Best of luck.
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....
I end up with 208 patches. I'm not sure that sending them all at once would be a good idea... julia
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_f... 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 <gustavoars@kernel.org> 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 <gustavoars@kernel.org> 1148 files changed, 2667 insertions(+), 2737 deletions(-)
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_f...
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 <gustavoars@kernel.org> 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 <gustavoars@kernel.org>
1148 files changed, 2667 insertions(+), 2737 deletions(-)
participants (3)
-
Joe Perches -
Julia Lawall -
Valdis Klētnieks