{"thread":{"id":"52243","subject":"[PATCH] coccinelle: merge two rules from flex_alloc.cocci","startedAt":"2019-11-12T15:34:39Z","lastAt":"2019-11-15T05:13:32Z","messageCount":9,"participants":["Markus Elfring","Denton Liu","Martin Ågren","SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"386020","messageId":"f867512c-e5b2-6bca-2a37-2976f4c182bd@web.de","threadId":"52243","inReplyTo":null,"subject":"[PATCH] coccinelle: merge two rules from flex_alloc.cocci","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-12T15:34:34Z","receivedAt":"2019-11-12T15:34:39Z","isPatch":true,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"From: Markus Elfring \nDate: Tue, 12 Nov 2019 16:30:14 +0100\n\nThis script contained two transformation rules for the semantic patch language\nwhich used duplicate code.\nThus combine these rules by using a SmPL disjunction for the replacement\nof two identifiers.\n\nSigned-off-by: Markus Elfring \n---\n contrib/coccinelle/flex_alloc.cocci | 25 +++++++++++++------------\n 1 file changed, 13 insertions(+), 12 deletions(-)\n\ndiff --git a/contrib/coccinelle/flex_alloc.cocci b/contrib/coccinelle/flex_alloc.cocci\nindex e9f7f6d861..1b4fa8f801 100644\n--- a/contrib/coccinelle/flex_alloc.cocci\n+++ b/contrib/coccinelle/flex_alloc.cocci\n@@ -1,13 +1,14 @@\n-@@\n+@adjustment@\n expression str;\n-identifier x, flexname;\n-@@\n-- FLEX_ALLOC_MEM(x, flexname, str, strlen(str));\n-+ FLEX_ALLOC_STR(x, flexname, str);\n-\n-@@\n-expression str;\n-identifier x, ptrname;\n-@@\n-- FLEXPTR_ALLOC_MEM(x, ptrname, str, strlen(str));\n-+ FLEXPTR_ALLOC_STR(x, ptrname, str);\n+identifier x, name;\n+@@\n+(\n+-FLEX_ALLOC_MEM\n++FLEX_ALLOC_STR\n+|\n+-FLEXPTR_ALLOC_MEM\n++FLEXPTR_ALLOC_STR\n+)\n+ (x, name, str\n+- , strlen(str)\n+ );\n--\n2.24.0\n\n"},{"id":"386026","messageId":"20191112175926.GA41101@generichostname","threadId":"52243","inReplyTo":"f867512c-e5b2-6bca-2a37-2976f4c182bd@web.de","subject":"Re: [PATCH] coccinelle: merge two rules from flex_alloc.cocci","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-11-12T17:59:26Z","receivedAt":"2019-11-12T17:59:31Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Markus,\n\nThanks for the contribution.\n\nI see that you've sent many Coccinelle patches to the mailing list. It\nmight be better to send them all together as a single threaded patchset\nso that reviewers will have an easier time finding all of them.\n\nOn Tue, Nov 12, 2019 at 04:34:34PM +0100, Markus Elfring wrote:\n> From: Markus Elfring \n> Date: Tue, 12 Nov 2019 16:30:14 +0100\n> \n> This script contained two transformation rules for the semantic patch language\n> which used duplicate code.\n> Thus combine these rules by using a SmPL disjunction for the replacement\n> of two identifiers.\n> \n> Signed-off-by: Markus Elfring \n> ---\n> contrib/coccinelle/flex_alloc.cocci | 25 +++++++++++++------------\n> 1 file changed, 13 insertions(+), 12 deletions(-)\n> \n> diff --git a/contrib/coccinelle/flex_alloc.cocci b/contrib/coccinelle/flex_alloc.cocci\n> index e9f7f6d861..1b4fa8f801 100644\n> --- a/contrib/coccinelle/flex_alloc.cocci\n> +++ b/contrib/coccinelle/flex_alloc.cocci\n> @@ -1,13 +1,14 @@\n> -@@\n> +@adjustment@\n\nNone of our other cocci scripts have rulenames so I would drop the\nrulename here. It also doesn't really help since its name is so generic.\n\nI would also echo this for the other patches you've sent.\n\n> expression str;\n> -identifier x, flexname;\n> -@@\n> -- FLEX_ALLOC_MEM(x, flexname, str, strlen(str));\n> -+ FLEX_ALLOC_STR(x, flexname, str);\n> -\n> -@@\n> -expression str;\n> -identifier x, ptrname;\n> -@@\n> -- FLEXPTR_ALLOC_MEM(x, ptrname, str, strlen(str));\n> -+ FLEXPTR_ALLOC_STR(x, ptrname, str);\n> +identifier x, name;\n> +@@\n> +(\n> +-FLEX_ALLOC_MEM\n> ++FLEX_ALLOC_STR\n> +|\n> +-FLEXPTR_ALLOC_MEM\n> ++FLEXPTR_ALLOC_STR\n> +)\n> + (x, name, str\n> +- , strlen(str)\n> + );\n\nSmall nitpick but to be inline with how the rest of our cocci scripts\nare written, I'd write this as\n\n\t (x, name, str\n\t- , strlen(str)\n\t );\n\nThanks,\n\nDenton\n\n> --\n> 2.24.0\n> \n"},{"id":"386128","messageId":"CAN0heSodNonkDK8AT9iJqmWLLCdO0OoHho0ijZOAmri5ren2dw@mail.gmail.com","threadId":"52243","inReplyTo":"20191112175926.GA41101@generichostname","subject":"Re: [PATCH] coccinelle: merge two rules from flex_alloc.cocci","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-13T18:27:23Z","receivedAt":"2019-11-13T18:27:37Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Markus,\n\nOn Tue, 12 Nov 2019 at 19:02, Denton Liu wrote:\n> I see that you've sent many Coccinelle patches to the mailing list. It\n> might be better to send them all together as a single threaded patchset\n> so that reviewers will have an easier time finding all of them.\n\nFWIW, I had the same thought.\n\n> > This script contained two transformation rules for the semantic patch language\n> > which used duplicate code.\n> > Thus combine these rules by using a SmPL disjunction for the replacement\n> > of two identifiers.\n\nMy knowledge of coccinelle and cocci rules is basically zero, but my\nimpression from this list is that running \"make coccicheck\" can be\nexpensive, both in terms of time and memory. Do these patches help there\nin any way? Or could they hurt?\n\nMartin\n"},{"id":"386143","messageId":"ff240bc1-ae2a-17e5-d149-2d08c5367e96@web.de","threadId":"52243","inReplyTo":"CAN0heSodNonkDK8AT9iJqmWLLCdO0OoHho0ijZOAmri5ren2dw@mail.gmail.com","subject":"Re: coccinelle: merge two rules from flex_alloc.cocci","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-13T21:10:48Z","receivedAt":"2019-11-13T21:11:00Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":">>> This script contained two transformation rules for the semantic patch language\n>>> which used duplicate code.\n>>> Thus combine these rules by using a SmPL disjunction for the replacement\n>>> of two identifiers.\n>\n> My knowledge of coccinelle and cocci rules is basically zero,\n\nWould you like to change this situation eventually?\n\n\n> but my impression from this list is that running \"make coccicheck\"\n> can be expensive, both in terms of time and memory.\n\nThe desired source code analysis to detect possible software transformations\nneeds additional data processing resources.\nIt is usually hoped that corresponding efforts will help with development\napproaches at other places.\n\n\n> Do these patches help there in any way?\n\nI hope so to some degree.\n\nHow much do you care to avoid code duplication?\n\n\n> Or could they hurt?\n\nI assume that you ask according to the presented change possibilities\nfor Git's SmPL scripts (and not only for “flex_alloc.cocci”).\n\nSome changes usually contain the risk for undesirable effects.\nWould you like to clarify each of them in more detail?\n\nRegards,\nMarkus\n"},{"id":"386169","messageId":"CAN0heSqyGwkeGKv0m_gLDooaUp=gN2_tD7kJYNxeL7LALiPRhQ@mail.gmail.com","threadId":"52243","inReplyTo":"ff240bc1-ae2a-17e5-d149-2d08c5367e96@web.de","subject":"Re: coccinelle: merge two rules from flex_alloc.cocci","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2019-11-14T06:37:20Z","receivedAt":"2019-11-14T06:37:34Z","isPatch":false,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 13 Nov 2019 at 22:10, Markus Elfring wrote:\n>\n> >>> This script contained two transformation rules for the semantic patch language\n> >>> which used duplicate code.\n> >>> Thus combine these rules by using a SmPL disjunction for the replacement\n> >>> of two identifiers.\n> >\n> > My knowledge of coccinelle and cocci rules is basically zero,\n>\n> Would you like to change this situation eventually?\n\nPossibly, yeah, but I think the key word there is \"eventually\". ;-)\nBut maybe I'll learn something from this exchange.\n\n> > but my impression from this list is that running \"make coccicheck\"\n> > can be expensive, both in terms of time and memory.\n>\n> The desired source code analysis to detect possible software transformations\n> needs additional data processing resources.\n> It is usually hoped that corresponding efforts will help with development\n> approaches at other places.\n\nRight. So by that logic, if this patch doubles the memory usage and/or time\nconsumption of \"make coccicheck\", wouldn't this patch be affecting those\nother activities at other places of the code negatively? ;-)\n\nI'm not saying that this patch DOES affect the time/memory usage\nnegatively, and I'm not saying this patch IS a net negative. Definitely\nnot. It's just that considering, e.g., 960154b9c1 (\"coccicheck:\noptionally batch spatch invocations\", 2019-05-06), time/memory\nconsumption -- and the balancing of the two -- seems to be an actual\nreal-world issue here, worth thinking about.\n\n(That commit message mentions that processing all source files in one go\nrequires close to 2GB of memory. We've since started processing more\nfiles.)\n\n> > Do these patches help there in any way?\n>\n> I hope so to some degree.\n\nIf you could have some before/after numbers, that would be cool. If you\ncollect your patches into one series, you could at least do measurements\nbefore/after the series.\n\nOr if you could make some other sort of claim around \"this shouldn't\naffect this-or-that because so-and-so\".\n\n> How much do you care to avoid code duplication?\n\nI tend to like it, everything else equal.\n\n> > Or could they hurt?\n>\n> I assume that you ask according to the presented change possibilities\n> for Git's SmPL scripts (and not only for “flex_alloc.cocci”).\n>\n> Some changes usually contain the risk for undesirable effects.\n> Would you like to clarify each of them in more detail?\n\nDo you mean whether I would like to clarify the risks I see, or do you\nmean whether I would like you to clarify which you see? I've tried to\nclarify the one I see -- based on passively observing cocci-related\npatches floating around this list. If you see other potential risks,\nfeel free to mention them.\n\nYou seem to know lots more than I do about these things. I wouldn't be\nsurprised if you know more on this than most or all other participants\non this list, so feel free to share some of that in the commit messages\nso that others can understand how you've reasoned.\n\nMartin\n"},{"id":"386175","messageId":"1d08b49e-1f41-4290-a64b-dad9fd2288de@web.de","threadId":"52243","inReplyTo":"CAN0heSqyGwkeGKv0m_gLDooaUp=gN2_tD7kJYNxeL7LALiPRhQ@mail.gmail.com","subject":"Re: coccinelle: merge two rules from flex_alloc.cocci","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-14T08:15:47Z","receivedAt":"2019-11-14T08:15:52Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":"> If you could have some before/after numbers, that would be cool.\n\nDoes any test infrastructure (or benchmarks) exist which you would trust for\ncorresponding comparisons of software run time characteristics?\n\n\n> If you collect your patches into one series, you could at least do measurements\n> before/after the series.\n\nHow do you think about to check possible improvements by each presented patch\naccording to affected SmPL scripts individually?\n\n\n> Or if you could make some other sort of claim around \"this shouldn't\n> affect this-or-that because so-and-so\".\n\nI try to avoid such claims.\nBut I provided specific information in my patch descriptions.\nDo you find any details reasonable there?\n\n\n> Do you mean whether I would like to clarify the risks I see, or do you\n> mean whether I would like you to clarify which you see?\n\nBoth. - The discussion will depend also on your change acceptance and desire\nto extend development in the shown software design directions.\n\n\n> I've tried to clarify the one I see -- based on passively observing cocci-related\n> patches floating around this list.\n\nThanks for your interest.\n\n\n> If you see other potential risks, feel free to mention them.\n\nI would prefer a more optimistic view while my software development\nexperiences can influence this considerably.\n\n\n> You seem to know lots more than I do about these things.\n\nThis can be. - But I hope that you can get further inspirations and ideas\nif you would find any of the published development activities interesting enough.\n\nRegards,\nMarkus\n"},{"id":"386186","messageId":"20191114163527.GT4348@szeder.dev","threadId":"52243","inReplyTo":"1d08b49e-1f41-4290-a64b-dad9fd2288de@web.de","subject":"Re: coccinelle: merge two rules from flex_alloc.cocci","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-14T16:35:27Z","receivedAt":"2019-11-14T16:35:37Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Nov 14, 2019 at 09:15:47AM +0100, Markus Elfring wrote:\n> > If you could have some before/after numbers, that would be cool.\n> \n> Does any test infrastructure (or benchmarks) exist which you would trust for\n> corresponding comparisons of software run time characteristics?\n\nYes, just run:\n\n make cocciclean\n time make contrib/coccinelle/flex_alloc.cocci.patch\n\nbefore and after your changes, and include the timing results in the\ncommit message if there is a notable difference. If it gets faster,\ngreat! If it gets slower, then update the commit message with a\nconvincing argument about why the change is worth the performance\npenalty.\n\nFWIW, I did just that with your \"coccinelle: merge twelve rules from\nobject_id.cocci\" patch [1], and the runtime went down from 2m48.610 to\n2m34.395, a bit over 8% speedup. (with Ubuntu 16.04's Coccinelle\n1.0.4)\n\n\n[1] https://public-inbox.org/git/6c9962c0-67c1-e700-c145-793ce6498099@web.de/\n\n"},{"id":"386194","messageId":"751e4cc2-e6d0-edfb-b206-4204443a5c62@web.de","threadId":"52243","inReplyTo":"20191114163527.GT4348@szeder.dev","subject":"Re: coccinelle: merge two rules from flex_alloc.cocci","fromName":"Markus Elfring","fromEmail":"markus.elfring@web.de","sentAt":"2019-11-14T17:30:24Z","receivedAt":"2019-11-14T17:30:28Z","isPatch":false,"sender":{"key":"markus.elfring@web.de","avatar":null},"body":">> Does any test infrastructure (or benchmarks) exist which you would trust for\n>> corresponding comparisons of software run time characteristics?\n>\n> Yes, just run:\n>\n> make cocciclean\n> time make contrib/coccinelle/flex_alloc.cocci.patch\n\nThanks for this suggestion.\n\n\n> before and after your changes, and include the timing results in the\n> commit message if there is a notable difference.\n\nThe measurements from my test system might not be representative enough.\n\n\n> FWIW, I did just that with your \"coccinelle: merge twelve rules from\n> object_id.cocci\" patch [1], and the runtime went down from 2m48.610 to\n> 2m34.395, a bit over 8% speedup. (with Ubuntu 16.04's Coccinelle\n> 1.0.4)\n>\n>\n> [1] https://public-inbox.org/git/6c9962c0-67c1-e700-c145-793ce6498099@web.de/\n\nThese numbers seem to be nice.\nWould a direct reply have been more helpful for the referenced patch\nthan the current subject?\n\nRegards,\nMarkus\n"},{"id":"386254","messageId":"xmqq5zjl90y3.fsf@gitster-ct.c.googlers.com","threadId":"52243","inReplyTo":"20191114163527.GT4348@szeder.dev","subject":"Re: coccinelle: merge two rules from flex_alloc.cocci","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-15T05:13:24Z","receivedAt":"2019-11-15T05:13:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Thu, Nov 14, 2019 at 09:15:47AM +0100, Markus Elfring wrote:\n>> > If you could have some before/after numbers, that would be cool.\n>> \n>> Does any test infrastructure (or benchmarks) exist which you would trust for\n>> corresponding comparisons of software run time characteristics?\n>\n> Yes, just run:\n>\n> make cocciclean\n> time make contrib/coccinelle/flex_alloc.cocci.patch\n>\n> before and after your changes, and include the timing results in the\n> commit message if there is a notable difference. If it gets faster,\n> great! If it gets slower, then update the commit message with a\n> convincing argument about why the change is worth the performance\n> penalty.\n\nAlso, the contents of the *.patch file before and after the change\nneeds to match, but for that test to be meaningful, you'd need to\nresurrect the problems we fixed with the help of the existing cocci\nscripts (otherwise, the resulting *.patch file being empty does not\nprove much---our source may now be too clean to demonstrate the\ndifference of the before/after rules).\n\nThanks.\n"}]}