Kernel Newbies Archive on lore.kernel.org
 help / color / Atom feed
* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
       [not found]         ` <alpine.DEB.2.22.394.2008201856110.2524@hadrien>
@ 2020-08-22  1:08           ` Joe Perches
  2020-08-22  3:35             ` Valdis Klētnieks
  0 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2020-08-22  1:08 UTC (permalink / raw)
  To: Julia Lawall, kernel-janitors, kernelnewbies, linux-kernel-mentees
  Cc: Giuseppe Scrivano, Andrew Morton, Andy Whitcroft, cocci, LKML

(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...



_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-08-22  1:08           ` [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon) Joe Perches
@ 2020-08-22  3:35             ` Valdis Klētnieks
  2020-08-22  5:30               ` Joe Perches
  0 siblings, 1 reply; 15+ messages in thread
From: Valdis Klētnieks @ 2020-08-22  3:35 UTC (permalink / raw)
  To: Joe Perches
  Cc: Giuseppe Scrivano, kernelnewbies, kernel-janitors, LKML,
	Julia Lawall, Andy Whitcroft, Andrew Morton,
	linux-kernel-mentees, cocci

[-- Attachment #1.1: Type: text/plain, Size: 838 bytes --]

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 {}.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-08-22  3:35             ` Valdis Klētnieks
@ 2020-08-22  5:30               ` Joe Perches
  2020-08-22  7:07                 ` Julia Lawall
  0 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2020-08-22  5:30 UTC (permalink / raw)
  To: Valdis Klētnieks
  Cc: Giuseppe Scrivano, kernelnewbies, kernel-janitors, LKML,
	Julia Lawall, Andy Whitcroft, Andrew Morton,
	linux-kernel-mentees, cocci

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.



_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-08-22  5:30               ` Joe Perches
@ 2020-08-22  7:07                 ` Julia Lawall
  2020-09-24 20:19                   ` Thomas Gleixner
  0 siblings, 1 reply; 15+ messages in thread
From: Julia Lawall @ 2020-08-22  7:07 UTC (permalink / raw)
  To: Joe Perches
  Cc: Giuseppe Scrivano, Valdis Klētnieks, kernelnewbies,
	kernel-janitors, LKML, Andy Whitcroft, Andrew Morton,
	linux-kernel-mentees, cocci


[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]



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

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-08-22  7:07                 ` Julia Lawall
@ 2020-09-24 20:19                   ` Thomas Gleixner
  2020-09-24 20:21                     ` Julia Lawall
  2020-09-24 20:33                     ` Joe Perches
  0 siblings, 2 replies; 15+ messages in thread
From: Thomas Gleixner @ 2020-09-24 20:19 UTC (permalink / raw)
  To: Julia Lawall, Joe Perches
  Cc: Giuseppe Scrivano, Valdis Klētnieks, kernelnewbies,
	kernel-janitors, LKML, Andy Whitcroft, Andrew Morton,
	linux-kernel-mentees, cocci

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

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-09-24 20:19                   ` Thomas Gleixner
@ 2020-09-24 20:21                     ` Julia Lawall
  2020-09-24 20:33                     ` Joe Perches
  1 sibling, 0 replies; 15+ messages in thread
From: Julia Lawall @ 2020-09-24 20:21 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Giuseppe Scrivano, Valdis Klētnieks, kernelnewbies,
	kernel-janitors, LKML, Andy Whitcroft, Joe Perches,
	Andrew Morton, linux-kernel-mentees, cocci



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
>

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-09-24 20:19                   ` Thomas Gleixner
  2020-09-24 20:21                     ` Julia Lawall
@ 2020-09-24 20:33                     ` Joe Perches
  2020-09-24 21:53                       ` Thomas Gleixner
  1 sibling, 1 reply; 15+ messages in thread
From: Joe Perches @ 2020-09-24 20:33 UTC (permalink / raw)
  To: Thomas Gleixner, Julia Lawall
  Cc: Giuseppe Scrivano, Valdis Klētnieks, kernelnewbies,
	kernel-janitors, LKML, Andy Whitcroft, Andrew Morton,
	linux-kernel-mentees, cocci

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...



_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-09-24 21:53                       ` Thomas Gleixner
@ 2020-09-24 22:23                         ` Joe Perches
  2020-09-25 17:06                           ` Julia Lawall
  2020-09-27 17:08                           ` Julia Lawall
  0 siblings, 2 replies; 15+ messages in thread
From: Joe Perches @ 2020-09-24 22:23 UTC (permalink / raw)
  To: Thomas Gleixner, Julia Lawall
  Cc: Giuseppe Scrivano, Valdis Klētnieks, kernelnewbies,
	kernel-janitors, LKML, Andy Whitcroft, Andrew Morton,
	linux-kernel-mentees, cocci

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



_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-09-24 22:23                         ` Joe Perches
@ 2020-09-25 17:06                           ` Julia Lawall
  2020-09-25 17:26                             ` Joe Perches
  2020-09-27 17:08                           ` Julia Lawall
  1 sibling, 1 reply; 15+ messages in thread
From: Julia Lawall @ 2020-09-25 17:06 UTC (permalink / raw)
  To: Joe Perches
  Cc: Giuseppe Scrivano, Valdis Klētnieks, kernelnewbies,
	kernel-janitors, LKML, cocci, Andy Whitcroft, Thomas Gleixner,
	linux-kernel-mentees, Andrew Morton



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

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-09-25 17:06                           ` Julia Lawall
@ 2020-09-25 17:26                             ` Joe Perches
  2020-09-26 19:11                               ` Valdis Klētnieks
  0 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2020-09-25 17:26 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Giuseppe Scrivano, Valdis Klētnieks, kernelnewbies,
	kernel-janitors, LKML, cocci, Andy Whitcroft, Thomas Gleixner,
	linux-kernel-mentees, Andrew Morton

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.



_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-09-25 17:26                             ` Joe Perches
@ 2020-09-26 19:11                               ` Valdis Klētnieks
  0 siblings, 0 replies; 15+ messages in thread
From: Valdis Klētnieks @ 2020-09-26 19:11 UTC (permalink / raw)
  To: Joe Perches
  Cc: Giuseppe Scrivano, kernelnewbies, kernel-janitors, LKML, cocci,
	Julia Lawall, Andy Whitcroft, Thomas Gleixner,
	linux-kernel-mentees, Andrew Morton

[-- Attachment #1.1: Type: text/plain, Size: 1861 bytes --]

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....



[-- Attachment #1.2: Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-09-24 22:23                         ` Joe Perches
  2020-09-25 17:06                           ` Julia Lawall
@ 2020-09-27 17:08                           ` Julia Lawall
  2020-09-27 17:45                             ` Joe Perches
  1 sibling, 1 reply; 15+ messages in thread
From: Julia Lawall @ 2020-09-27 17:08 UTC (permalink / raw)
  To: Joe Perches
  Cc: Giuseppe Scrivano, Valdis Klētnieks, kernelnewbies,
	kernel-janitors, LKML, cocci, Andy Whitcroft, Thomas Gleixner,
	linux-kernel-mentees, Andrew Morton

I end up with 208 patches.  I'm not sure that sending them all at once
would be a good idea...

julia

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-09-27 17:08                           ` Julia Lawall
@ 2020-09-27 17:45                             ` Joe Perches
  2020-09-27 19:35                               ` Julia Lawall
  0 siblings, 1 reply; 15+ messages in thread
From: Joe Perches @ 2020-09-27 17:45 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Giuseppe Scrivano, Gustavo A. R. Silva, Valdis Klētnieks,
	lkp, kernelnewbies, kernel-janitors, LKML, cocci, Andy Whitcroft,
	Thomas Gleixner, linux-kernel-mentees, Andrew Morton

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 <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(-)



_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-09-27 17:45                             ` Joe Perches
@ 2020-09-27 19:35                               ` Julia Lawall
  0 siblings, 0 replies; 15+ messages in thread
From: Julia Lawall @ 2020-09-27 19:35 UTC (permalink / raw)
  To: Joe Perches
  Cc: Giuseppe Scrivano, Gustavo A. R. Silva, Valdis Klētnieks,
	lkp, kernelnewbies, kernel-janitors, LKML, cocci, Andy Whitcroft,
	Thomas Gleixner, linux-kernel-mentees, Andrew Morton



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 <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(-)
>
>
>

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon)
  2020-09-24 20:33                     ` Joe Perches
@ 2020-09-24 21:53                       ` Thomas Gleixner
  2020-09-24 22:23                         ` Joe Perches
  0 siblings, 1 reply; 15+ messages in thread
From: Thomas Gleixner @ 2020-09-24 21:53 UTC (permalink / raw)
  To: Joe Perches, Julia Lawall
  Cc: Giuseppe Scrivano, Valdis Klētnieks, kernelnewbies,
	kernel-janitors, LKML, Andy Whitcroft, Andrew Morton,
	linux-kernel-mentees, cocci

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

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200818184107.f8af232fb58b17160c570874@linux-foundation.org>
     [not found] ` <3bf27caf462007dfa75647b040ab3191374a59de.camel@perches.com>
     [not found]   ` <b0fd63e4b379eda69ee85ab098353582b8c054bb.camel@perches.com>
     [not found]     ` <alpine.DEB.2.22.394.2008201021400.2524@hadrien>
     [not found]       ` <a5713d8597065ef986f780499428fcc4cd31c003.camel@perches.com>
     [not found]         ` <alpine.DEB.2.22.394.2008201856110.2524@hadrien>
2020-08-22  1:08           ` [Cocci] coccinelle: Convert comma to semicolons (was Re: [PATCH] checkpatch: Add test for comma use that should be semicolon) Joe Perches
2020-08-22  3:35             ` Valdis Klētnieks
2020-08-22  5:30               ` Joe Perches
2020-08-22  7:07                 ` Julia Lawall
2020-09-24 20:19                   ` Thomas Gleixner
2020-09-24 20:21                     ` Julia Lawall
2020-09-24 20:33                     ` Joe Perches
2020-09-24 21:53                       ` Thomas Gleixner
2020-09-24 22:23                         ` Joe Perches
2020-09-25 17:06                           ` Julia Lawall
2020-09-25 17:26                             ` Joe Perches
2020-09-26 19:11                               ` Valdis Klētnieks
2020-09-27 17:08                           ` Julia Lawall
2020-09-27 17:45                             ` Joe Perches
2020-09-27 19:35                               ` Julia Lawall

Kernel Newbies Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kernelnewbies/0 kernelnewbies/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kernelnewbies kernelnewbies/ https://lore.kernel.org/kernelnewbies \
		kernelnewbies@kernelnewbies.org
	public-inbox-index kernelnewbies

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernelnewbies.kernelnewbies


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git