Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script
On Tue, 29 Oct 2019, Masahiro Yamada wrote: > Hi Julia > > On Fri, Oct 25, 2019 at 5:38 PM Julia Lawall wrote: > > > > > > > > On Fri, 25 Oct 2019, Andy Shevchenko wrote: > > > > > On Fri, Oct 25, 2019 at 12:40:52AM +0900, Masahiro Yamada wrote: > > > > On Sun, Oct 20, 2019 at 7:13 AM Joe Perches wrote: > > > > > On Sat, 2019-10-19 at 21:43 +0100, Marc Zyngier wrote: > > > > > > > Alexandre Belloni used > > > > https://lore.kernel.org/lkml/9bbcce19c777583815c92ce3c2ff2...@www.loen.fr/ > > > > as a reference, but this is not the output from coccicheck. > > > > The patch author just created a wrong patch by hand. > > > > > > Exactly. Removal of the script is a mistake. Like I said before is a > > > healing > > > (incorrect by the way!) by symptoms. > > > > > > > The deleted semantic patch supports MODE=patch, > > > > which creates a correct patch, and is useful. > > > > > > Right! > > > > I ran it on the version of Linux that still has the script: > > > > fe7d2c23d748e4206f4bef9330d0dff9abed7411 > > > > and managed to compile 341 of the generated files in the time I had > > available, and all compiled successfully. > > Yeah, this semantic patch did the correct conversion > as its header part showed the confidence. > > // Confidence: High > > > > > I can let it run again, and see > > how it goes for the rest. Perhaps it would be acceptable if there was no > > report, and people would be forced to use the generated patch? > > I do not think this is the right thing. > MODE=report is the default, and it is fine. > > > > > If someone is writing lots of patches on this issue by hand, then perhaps > > they don't have make coccicheck to produce patches, and then would > > overlook this case completely. > > > > If it would be helpful, I could group the generated patches by maintainer > > or by subdirectory and send them out, if it would be easier to review them > > all at once. > > Yes, please. > > Subsystem maintainers trust you, > so I think it will make things move smoothly. > > After converting most of files, > I want 283ea345934d277e30c841c577e0e2142b4bfcae reverted. OK. I got 477 of the files to compile directly. I can send patches on them, and then look into the issues on the remaining ones (probably configuration issues). julia > > > > > > Anyway, the rule is not in the kernel at the moment. For it's future, I'm > > open to whatever people find best. Personally, I prefer when same things > > are done in the same way - it makes the code easier to understand and > > makes it simpler to address other issues when they arise. > > > We always did the same things in the same way > except commit 283ea345934d277e30c841c577e0e2142b4bfcae > > > > > -- > Best Regards > Masahiro Yamada > ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script
Hi Julia On Fri, Oct 25, 2019 at 5:38 PM Julia Lawall wrote: > > > > On Fri, 25 Oct 2019, Andy Shevchenko wrote: > > > On Fri, Oct 25, 2019 at 12:40:52AM +0900, Masahiro Yamada wrote: > > > On Sun, Oct 20, 2019 at 7:13 AM Joe Perches wrote: > > > > On Sat, 2019-10-19 at 21:43 +0100, Marc Zyngier wrote: > > > > > Alexandre Belloni used > > > https://lore.kernel.org/lkml/9bbcce19c777583815c92ce3c2ff2...@www.loen.fr/ > > > as a reference, but this is not the output from coccicheck. > > > The patch author just created a wrong patch by hand. > > > > Exactly. Removal of the script is a mistake. Like I said before is a healing > > (incorrect by the way!) by symptoms. > > > > > The deleted semantic patch supports MODE=patch, > > > which creates a correct patch, and is useful. > > > > Right! > > I ran it on the version of Linux that still has the script: > > fe7d2c23d748e4206f4bef9330d0dff9abed7411 > > and managed to compile 341 of the generated files in the time I had > available, and all compiled successfully. Yeah, this semantic patch did the correct conversion as its header part showed the confidence. // Confidence: High > I can let it run again, and see > how it goes for the rest. Perhaps it would be acceptable if there was no > report, and people would be forced to use the generated patch? I do not think this is the right thing. MODE=report is the default, and it is fine. > > If someone is writing lots of patches on this issue by hand, then perhaps > they don't have make coccicheck to produce patches, and then would > overlook this case completely. > > If it would be helpful, I could group the generated patches by maintainer > or by subdirectory and send them out, if it would be easier to review them > all at once. Yes, please. Subsystem maintainers trust you, so I think it will make things move smoothly. After converting most of files, I want 283ea345934d277e30c841c577e0e2142b4bfcae reverted. > > Anyway, the rule is not in the kernel at the moment. For it's future, I'm > open to whatever people find best. Personally, I prefer when same things > are done in the same way - it makes the code easier to understand and > makes it simpler to address other issues when they arise. We always did the same things in the same way except commit 283ea345934d277e30c841c577e0e2142b4bfcae -- Best Regards Masahiro Yamada ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script
On Fri, Oct 25, 2019 at 12:40:52AM +0900, Masahiro Yamada wrote: > On Sun, Oct 20, 2019 at 7:13 AM Joe Perches wrote: > > On Sat, 2019-10-19 at 21:43 +0100, Marc Zyngier wrote: > Alexandre Belloni used > https://lore.kernel.org/lkml/9bbcce19c777583815c92ce3c2ff2...@www.loen.fr/ > as a reference, but this is not the output from coccicheck. > The patch author just created a wrong patch by hand. Exactly. Removal of the script is a mistake. Like I said before is a healing (incorrect by the way!) by symptoms. > The deleted semantic patch supports MODE=patch, > which creates a correct patch, and is useful. Right! -- With Best Regards, Andy Shevchenko ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script
On Fri, 25 Oct 2019, Andy Shevchenko wrote: > On Fri, Oct 25, 2019 at 12:40:52AM +0900, Masahiro Yamada wrote: > > On Sun, Oct 20, 2019 at 7:13 AM Joe Perches wrote: > > > On Sat, 2019-10-19 at 21:43 +0100, Marc Zyngier wrote: > > > Alexandre Belloni used > > https://lore.kernel.org/lkml/9bbcce19c777583815c92ce3c2ff2...@www.loen.fr/ > > as a reference, but this is not the output from coccicheck. > > The patch author just created a wrong patch by hand. > > Exactly. Removal of the script is a mistake. Like I said before is a healing > (incorrect by the way!) by symptoms. > > > The deleted semantic patch supports MODE=patch, > > which creates a correct patch, and is useful. > > Right! I ran it on the version of Linux that still has the script: fe7d2c23d748e4206f4bef9330d0dff9abed7411 and managed to compile 341 of the generated files in the time I had available, and all compiled successfully. I can let it run again, and see how it goes for the rest. Perhaps it would be acceptable if there was no report, and people would be forced to use the generated patch? If someone is writing lots of patches on this issue by hand, then perhaps they don't have make coccicheck to produce patches, and then would overlook this case completely. If it would be helpful, I could group the generated patches by maintainer or by subdirectory and send them out, if it would be easier to review them all at once. Anyway, the rule is not in the kernel at the moment. For it's future, I'm open to whatever people find best. Personally, I prefer when same things are done in the same way - it makes the code easier to understand and makes it simpler to address other issues when they arise. julia ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script
> I always appreciate the code refactoring > that reduces the object size. Would you like to compare effects around conversions for the mentioned wrapper function any more? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script
On Sun, Oct 20, 2019 at 7:13 AM Joe Perches wrote: > > On Sat, 2019-10-19 at 21:43 +0100, Marc Zyngier wrote: > > Providing Coccinelle scripts that scream about perfectly valid code is > > pointless, and the result is actively harmful. > > Doubtful. > > If the new code is smaller object code and correct > than the conversion is worthwhile. I agree. We use multi-platform defconfig. I always appreciate the code refactoring that reduces the object size. > fyi: > > There are already ~450 uses of this function and maybe > ~800 possible additional conversions. > > > If said script was providing a correct semantic patch instead of being > > an incentive for people to churn untested patches that span the whole > > tree, that'd be a different story. > > Right. > > Alexandre Belloni used https://lore.kernel.org/lkml/9bbcce19c777583815c92ce3c2ff2...@www.loen.fr/ as a reference, but this is not the output from coccicheck. The patch author just created a wrong patch by hand. The deleted semantic patch supports MODE=patch, which creates a correct patch, and is useful. -- Best Regards Masahiro Yamada ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script
> What are the additional effects?
I suggest to take another look at the commit
7945f929f1a77a1c8887a97ca07f87626858ff42
("drivers: provide devm_platform_ioremap_resource()" from 2019-02-20)
which triggered the discussed software evolution.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/base/platform.c
> What is the end goal of converting all the existing drivers to
> devm_platform_ioremap_resource?
It was accepted by well-known Linux developers to put two function calls
into another wrapper function.
> This is not an evolution, it is unnecessary churn. Those patches have no
> benefit and eat up very valuable reviewer time.
I am curious if other contributors would like to describe more variants
of software development opinions in affected areas.
>> How will such feedback influence the development and integration of
>> further scripts for the semantic patch language (Coccinelle software)?
>
> There are a few other scripts that have no added value when applied to
> existing code, like ptr_ret.cocci.
Would you like to clarify concerns around such source code transformation
approaches in more detail?
Regards,
Markus
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] coccinelle: api/devm_platform_ioremap_resource: remove useless script
> I think part of the issue is that the script reports a WARNING How much does this information influence really the stress tolerance and change resistance (or acceptance) for the presented collateral evolution? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/scripts/coccinelle/api/devm_platform_ioremap_resource.cocci > for something that is definitely correct code, Can related software improvement possibilities be taken into account again under other circumstances? > and could instead be simply toned down. Does this view mean that the mentioned script for the semantic patch language should get another chance for integration? > Anyway, FWIW: > > Acked-by: Marc Zyngier Would you like to share any more constructive feedback? Will similar source file mass updates be better picked up by other well-known Linux developers? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
