{"thread":{"id":"49152","subject":"[PATCH] gpg-interface.c: Fix potentially freeing NULL values","startedAt":"2018-08-17T09:17:20Z","lastAt":"2018-08-17T18:29:49Z","messageCount":12,"participants":["Michał Górny","Eric Sunshine","Ævar Arnfjörð Bjarmason","Duy Nguyen","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"355896","messageId":"20180817091710.1767-1-mgorny@gentoo.org","threadId":"49152","inReplyTo":null,"subject":"[PATCH] gpg-interface.c: Fix potentially freeing NULL values","fromName":"Michał Górny","fromEmail":"mgorny@gentoo.org","sentAt":"2018-08-17T09:17:10Z","receivedAt":"2018-08-17T09:17:20Z","isPatch":true,"sender":{"key":"mgorny@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/110765?v=4"},"body":"Fix signature_check_clear() to free only values that are non-NULL.  This\nespecially applies to 'key' and 'signer' members that can be NULL during\nnormal operations, depending on exact GnuPG output.  While at it, also\nallow other members to be NULL to make the function easier to use,\neven if there is no real need to account for that right now.\n\nSigned-off-by: Michał Górny <mgorny@gentoo.org>\n---\n gpg-interface.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 35c25106a..9aedaf464 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -15,9 +15,14 @@ static const char *gpg_program = \"gpg\";\n void signature_check_clear(struct signature_check *sigc)\n {\n-\tFREE_AND_NULL(sigc->payload);\n-\tFREE_AND_NULL(sigc->gpg_output);\n-\tFREE_AND_NULL(sigc->gpg_status);\n-\tFREE_AND_NULL(sigc->signer);\n-\tFREE_AND_NULL(sigc->key);\n+\tif (sigc->payload)\n+\t\tFREE_AND_NULL(sigc->payload);\n+\tif (sigc->gpg_output)\n+\t\tFREE_AND_NULL(sigc->gpg_output);\n+\tif (sigc->gpg_status)\n+\t\tFREE_AND_NULL(sigc->gpg_status);\n+\tif (sigc->signer)\n+\t\tFREE_AND_NULL(sigc->signer);\n+\tif (sigc->key)\n+\t\tFREE_AND_NULL(sigc->key);\n }\n \n-- \n2.18.0\n\n"},{"id":"355897","messageId":"CAPig+cQVWY3+2aarYw=uXti0=1SW8boMPoYj1zatw1KKKVOqnQ@mail.gmail.com","threadId":"49152","inReplyTo":"20180817091710.1767-1-mgorny@gentoo.org","subject":"Re: [PATCH] gpg-interface.c: Fix potentially freeing NULL values","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-17T09:28:54Z","receivedAt":"2018-08-17T09:29:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 17, 2018 at 5:17 AM Michał Górny <mgorny@gentoo.org> wrote:\n> Fix signature_check_clear() to free only values that are non-NULL.  This\n> especially applies to 'key' and 'signer' members that can be NULL during\n> normal operations, depending on exact GnuPG output.  While at it, also\n> allow other members to be NULL to make the function easier to use,\n> even if there is no real need to account for that right now.\n\nfree(NULL) is valid behavior[1] and much of the Git codebase relies upon it.\n\nDid you run into a case where it misbehaved?\n\n[1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/free.html\n\n> Signed-off-by: Michał Górny <mgorny@gentoo.org>\n> ---\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 35c25106a..9aedaf464 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -15,9 +15,14 @@ static const char *gpg_program = \"gpg\";\n>  void signature_check_clear(struct signature_check *sigc)\n>  {\n> -       FREE_AND_NULL(sigc->payload);\n> -       FREE_AND_NULL(sigc->gpg_output);\n> -       FREE_AND_NULL(sigc->gpg_status);\n> -       FREE_AND_NULL(sigc->signer);\n> -       FREE_AND_NULL(sigc->key);\n> +       if (sigc->payload)\n> +               FREE_AND_NULL(sigc->payload);\n> +       if (sigc->gpg_output)\n> +               FREE_AND_NULL(sigc->gpg_output);\n> +       if (sigc->gpg_status)\n> +               FREE_AND_NULL(sigc->gpg_status);\n> +       if (sigc->signer)\n> +               FREE_AND_NULL(sigc->signer);\n> +       if (sigc->key)\n> +               FREE_AND_NULL(sigc->key);\n>  }\n"},{"id":"355898","messageId":"1534498806.1262.8.camel@gentoo.org","threadId":"49152","inReplyTo":"CAPig+cQVWY3+2aarYw=uXti0=1SW8boMPoYj1zatw1KKKVOqnQ@mail.gmail.com","subject":"Re: [PATCH] gpg-interface.c: Fix potentially freeing NULL values","fromName":"Michał Górny","fromEmail":"mgorny@gentoo.org","sentAt":"2018-08-17T09:40:06Z","receivedAt":"2018-08-17T09:40:21Z","isPatch":true,"sender":{"key":"mgorny@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/110765?v=4"},"body":"On Fri, 2018-08-17 at 05:28 -0400, Eric Sunshine wrote:\n> On Fri, Aug 17, 2018 at 5:17 AM Michał Górny <mgorny@gentoo.org> wrote:\n> > Fix signature_check_clear() to free only values that are non-NULL.  This\n> > especially applies to 'key' and 'signer' members that can be NULL during\n> > normal operations, depending on exact GnuPG output.  While at it, also\n> > allow other members to be NULL to make the function easier to use,\n> > even if there is no real need to account for that right now.\n> \n> free(NULL) is valid behavior[1] and much of the Git codebase relies upon it.\n> \n> Did you run into a case where it misbehaved?\n\nNope.  I was actually wondering if it's expected, so I did a quick grep\nto check whether git is checking pointers for non-NULL before free()ing,\nand found at least one:\n\nblame.c-static void drop_origin_blob(struct blame_origin *o)\nblame.c-{\nblame.c-        if (o->file.ptr) {\nblame.c:                FREE_AND_NULL(o->file.ptr);\nblame.c-        }\nblame.c-}\n\nSo I wrongly presumed it might be desirable.  If it's not, that's fine\nby me.\n\n> \n> [1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/free.html\n> \n> > Signed-off-by: Michał Górny <mgorny@gentoo.org>\n> > ---\n> > diff --git a/gpg-interface.c b/gpg-interface.c\n> > index 35c25106a..9aedaf464 100644\n> > --- a/gpg-interface.c\n> > +++ b/gpg-interface.c\n> > @@ -15,9 +15,14 @@ static const char *gpg_program = \"gpg\";\n> >  void signature_check_clear(struct signature_check *sigc)\n> >  {\n> > -       FREE_AND_NULL(sigc->payload);\n> > -       FREE_AND_NULL(sigc->gpg_output);\n> > -       FREE_AND_NULL(sigc->gpg_status);\n> > -       FREE_AND_NULL(sigc->signer);\n> > -       FREE_AND_NULL(sigc->key);\n> > +       if (sigc->payload)\n> > +               FREE_AND_NULL(sigc->payload);\n> > +       if (sigc->gpg_output)\n> > +               FREE_AND_NULL(sigc->gpg_output);\n> > +       if (sigc->gpg_status)\n> > +               FREE_AND_NULL(sigc->gpg_status);\n> > +       if (sigc->signer)\n> > +               FREE_AND_NULL(sigc->signer);\n> > +       if (sigc->key)\n> > +               FREE_AND_NULL(sigc->key);\n> >  }\n\n-- \nBest regards,\nMichał Górny\n"},{"id":"355900","messageId":"20180817130250.20354-1-avarab@gmail.com","threadId":"49152","inReplyTo":"1534498806.1262.8.camel@gentoo.org","subject":"[PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-17T13:02:50Z","receivedAt":"2018-08-17T13:03:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the few conditional uses of FREE_AND_NULL(x) to be\nunconditional. As noted in the standard[1] free(NULL) is perfectly\nvalid, so we might as well leave this check up to the C library.\n\n1. http://pubs.opengroup.org/onlinepubs/9699919799/functions/free.html\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nLet's do the opposite of this instead.\n\n blame.c     | 4 +---\n branch.c    | 4 +---\n http.c      | 4 +---\n tree-diff.c | 4 +---\n 4 files changed, 4 insertions(+), 12 deletions(-)\n\ndiff --git a/blame.c b/blame.c\nindex 58a7036847..b22a95de7b 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -334,9 +334,7 @@ static void fill_origin_blob(struct diff_options *opt,\n \n static void drop_origin_blob(struct blame_origin *o)\n {\n-\tif (o->file.ptr) {\n-\t\tFREE_AND_NULL(o->file.ptr);\n-\t}\n+\tFREE_AND_NULL(o->file.ptr);\n }\n \n /*\ndiff --git a/branch.c b/branch.c\nindex ecd710d730..776f55fc66 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -25,9 +25,7 @@ static int find_tracked_branch(struct remote *remote, void *priv)\n \t\t\ttracking->remote = remote->name;\n \t\t} else {\n \t\t\tfree(tracking->spec.src);\n-\t\t\tif (tracking->src) {\n-\t\t\t\tFREE_AND_NULL(tracking->src);\n-\t\t\t}\n+\t\t\tFREE_AND_NULL(tracking->src);\n \t\t}\n \t\ttracking->spec.src = NULL;\n \t}\ndiff --git a/http.c b/http.c\nindex b4bfbceaeb..4162860ee3 100644\n--- a/http.c\n+++ b/http.c\n@@ -2418,9 +2418,7 @@ void release_http_object_request(struct http_object_request *freq)\n \t\tclose(freq->localfile);\n \t\tfreq->localfile = -1;\n \t}\n-\tif (freq->url != NULL) {\n-\t\tFREE_AND_NULL(freq->url);\n-\t}\n+\tFREE_AND_NULL(freq->url);\n \tif (freq->slot != NULL) {\n \t\tfreq->slot->callback_func = NULL;\n \t\tfreq->slot->callback_data = NULL;\ndiff --git a/tree-diff.c b/tree-diff.c\nindex fe2e466ac1..553bc0e63a 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -557,9 +557,7 @@ struct combine_diff_path *diff_tree_paths(\n \t * free pre-allocated last element, if any\n \t * (see path_appendnew() for details about why)\n \t */\n-\tif (p->next) {\n-\t\tFREE_AND_NULL(p->next);\n-\t}\n+\tFREE_AND_NULL(p->next);\n \n \treturn p;\n }\n-- \n2.18.0.865.gffc8e1a3cd6\n\n"},{"id":"355906","messageId":"CACsJy8DH2tESV4xkCYutH=Ye37zGwifGdJhdnNOsRd+JusdOwg@mail.gmail.com","threadId":"49152","inReplyTo":"20180817130250.20354-1-avarab@gmail.com","subject":"Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-17T14:36:13Z","receivedAt":"2018-08-17T14:36:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Aug 17, 2018 at 3:05 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> Change the few conditional uses of FREE_AND_NULL(x) to be\n> unconditional. As noted in the standard[1] free(NULL) is perfectly\n> valid, so we might as well leave this check up to the C library.\n\nI'm not trying to make you work more on this. But out of curiosity\nwould coccinelle help catch this pattern? Szeder's recent work on\nrunning cocci automatically would help catch all future code like this\nif we could write an spatch.\n-- \nDuy\n"},{"id":"355910","messageId":"20180817151012.GA20262@duynguyen.home","threadId":"49152","inReplyTo":"CACsJy8DH2tESV4xkCYutH=Ye37zGwifGdJhdnNOsRd+JusdOwg@mail.gmail.com","subject":"Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-17T15:10:12Z","receivedAt":"2018-08-17T15:10:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Aug 17, 2018 at 04:36:13PM +0200, Duy Nguyen wrote:\n> On Fri, Aug 17, 2018 at 3:05 PM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n> >\n> > Change the few conditional uses of FREE_AND_NULL(x) to be\n> > unconditional. As noted in the standard[1] free(NULL) is perfectly\n> > valid, so we might as well leave this check up to the C library.\n> \n> I'm not trying to make you work more on this. But out of curiosity\n> would coccinelle help catch this pattern? Szeder's recent work on\n> running cocci automatically would help catch all future code like this\n> if we could write an spatch.\n\nJust fyi this seems to do the trick. Although I'm nowhere good at\ncoccinelle to say if we should include this (or something like it)\n\n-- 8< --\ndiff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci\nindex 4490069df9..f8e018d104 100644\n--- a/contrib/coccinelle/free.cocci\n+++ b/contrib/coccinelle/free.cocci\n@@ -16,3 +16,9 @@ expression E;\n - free(E);\n + FREE_AND_NULL(E);\n - E = NULL;\n+\n+@@\n+expression E;\n+@@\n+- if (E) { FREE_AND_NULL(E); }\n++ FREE_AND_NULL(E);\n-- 8< --\n\n--\nDuy\n"},{"id":"355921","messageId":"xmqqpnyhaq93.fsf@gitster-ct.c.googlers.com","threadId":"49152","inReplyTo":"20180817151012.GA20262@duynguyen.home","subject":"Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-17T16:53:44Z","receivedAt":"2018-08-17T16:53:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> Just fyi this seems to do the trick. Although I'm nowhere good at\n> coccinelle to say if we should include this (or something like it)\n>\n> -- 8< --\n> diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci\n> index 4490069df9..f8e018d104 100644\n> --- a/contrib/coccinelle/free.cocci\n> +++ b/contrib/coccinelle/free.cocci\n> @@ -16,3 +16,9 @@ expression E;\n>  - free(E);\n>  + FREE_AND_NULL(E);\n>  - E = NULL;\n> +\n> +@@\n> +expression E;\n> +@@\n> +- if (E) { FREE_AND_NULL(E); }\n> ++ FREE_AND_NULL(E);\n\nIt is a bit sad that\n\n\t- if (E)\n\t  FREE_AND_NULL(E);\n\nis not sufficient to catch it.  Shouldn't we be doing the same for\nregular free(E) as well?  IOW, like the attached patch.\n\n contrib/coccinelle/free.cocci | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci\nindex 4490069df9..f748bcfe30 100644\n--- a/contrib/coccinelle/free.cocci\n+++ b/contrib/coccinelle/free.cocci\n@@ -16,3 +16,27 @@ expression E;\n - free(E);\n + FREE_AND_NULL(E);\n - E = NULL;\n+\n+@@\n+expression E;\n+@@\n+- if (E)\n+  FREE_AND_NULL(E);\n+\n+@@\n+expression E;\n+@@\n+- if (E) { free(E); }\n++ free(E);\n+\n+@@\n+expression E;\n+@@\n+- if (!E) { free(E); }\n++ free(E);\n+\n+@@\n+expression E;\n+@@\n+- if (E) { FREE_AND_NULL(E); }\n++ FREE_AND_NULL(E);\n\n\n"},{"id":"355922","messageId":"xmqqlg94c46f.fsf@gitster-ct.c.googlers.com","threadId":"49152","inReplyTo":"xmqqpnyhaq93.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-17T17:07:36Z","receivedAt":"2018-08-17T17:07:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> It is a bit sad that\n>\n> \t- if (E)\n> \t  FREE_AND_NULL(E);\n>\n> is not sufficient to catch it.  Shouldn't we be doing the same for\n> regular free(E) as well?  IOW, like the attached patch.\n> ...\n\nAnd revised even more to also spell \"E\" as \"E != NULL\" (and \"!E\" as\n\"E == NULL\"), which seems to make a difference, which is even more\nsad.  I do not want to wonder if I have to also add \"NULL == E\" and\nother variants, so I'll stop here.\n\n contrib/coccinelle/free.cocci | 60 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 60 insertions(+)\n\ndiff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci\nindex 4490069df9..29ca98796f 100644\n--- a/contrib/coccinelle/free.cocci\n+++ b/contrib/coccinelle/free.cocci\n@@ -16,3 +16,63 @@ expression E;\n - free(E);\n + FREE_AND_NULL(E);\n - E = NULL;\n+\n+@@\n+expression E;\n+@@\n+- if (E)\n+  FREE_AND_NULL(E);\n+\n+@@\n+expression E;\n+@@\n+- if (E != NULL)\n+  free(E);\n+\n+@@\n+expression E;\n+@@\n+- if (E == NULL)\n+  free(E);\n+\n+@@\n+expression E;\n+@@\n+- if (E != NULL)\n+  FREE_AND_NULL(E);\n+\n+@@\n+expression E;\n+@@\n+- if (E) { free(E); }\n++ free(E);\n+\n+@@\n+expression E;\n+@@\n+- if (!E) { free(E); }\n++ free(E);\n+\n+@@\n+expression E;\n+@@\n+- if (E) { FREE_AND_NULL(E); }\n++ FREE_AND_NULL(E);\n+\n+@@\n+expression E;\n+@@\n+- if (E != NULL) { free(E); }\n++ free(E);\n+\n+@@\n+expression E;\n+@@\n+- if (E == NULL) { free(E); }\n++ free(E);\n+\n+@@\n+expression E;\n+@@\n+- if (E != NULL) { FREE_AND_NULL(E); }\n++ FREE_AND_NULL(E);\n\n"},{"id":"355924","messageId":"20180817173308.GA9111@sigill.intra.peff.net","threadId":"49152","inReplyTo":"xmqqlg94c46f.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-17T17:33:08Z","receivedAt":"2018-08-17T17:33:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 17, 2018 at 10:07:36AM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > It is a bit sad that\n> >\n> > \t- if (E)\n> > \t  FREE_AND_NULL(E);\n> >\n> > is not sufficient to catch it.  Shouldn't we be doing the same for\n> > regular free(E) as well?  IOW, like the attached patch.\n> > ...\n> \n> And revised even more to also spell \"E\" as \"E != NULL\" (and \"!E\" as\n> \"E == NULL\"), which seems to make a difference, which is even more\n> sad.  I do not want to wonder if I have to also add \"NULL == E\" and\n> other variants, so I'll stop here.\n\nI think it makes sense that these are all distinct if you're using\ncoccinelle to do stylistic transformations between them (e.g., enforcing\ncurly braces even around one-liners).\n\nI wonder if there is a way to \"relax\" a pattern where these semantically\nequivalent cases can all be covered automatically. I don't know enough\nabout the tool to say.\n\nI guess one way to do it would be to normalize the style in one rule\n(e.g., always \"!E\" instead of \"E == NULL\"), and then you only have to\nwrite the FREE_AND_NULL rule for the normalized form. For a single case\nlike this, the end result is about the same number of rules, but in the\nlong term it saves us work when we have a similar transformation.\n\n-Peff\n"},{"id":"355926","messageId":"20180817174404.GB9474@sigill.intra.peff.net","threadId":"49152","inReplyTo":"20180817173951.GA9474@sigill.intra.peff.net","subject":"Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-17T17:44:05Z","receivedAt":"2018-08-17T17:44:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 17, 2018 at 01:39:51PM -0400, Jeff King wrote:\n\n> > I wonder if there is a way to \"relax\" a pattern where these semantically\n> > equivalent cases can all be covered automatically. I don't know enough\n> > about the tool to say.\n> \n> Hmm. They seem to call these \"standard isomorphisms\":\n> \n>   http://coccinelle.lip6.fr/standard.iso.html\n> \n> but I'm not sure of the correct way to use them (e.g., if we want to\n> apply them for matching but not actually transform the code, though I am\n> not actually opposed to transforming the code, too).\n\nHmph, I should really pause before hitting 'send'. Last message, I\npromise. :)\n\nI do not see an option to include a list an arbitrary set of\nisomorphisms, but the standard.iso list should be used by default. I\nwonder if you simply need to write your case in the normalized version\nthey use there (which I think is \"X == NULL\"), and the others would be\ntaken care of.\n\n-Peff\n"},{"id":"355927","messageId":"20180817173951.GA9474@sigill.intra.peff.net","threadId":"49152","inReplyTo":"20180817173308.GA9111@sigill.intra.peff.net","subject":"Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-17T17:39:51Z","receivedAt":"2018-08-17T17:46:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 17, 2018 at 01:33:08PM -0400, Jeff King wrote:\n\n> > And revised even more to also spell \"E\" as \"E != NULL\" (and \"!E\" as\n> > \"E == NULL\"), which seems to make a difference, which is even more\n> > sad.  I do not want to wonder if I have to also add \"NULL == E\" and\n> > other variants, so I'll stop here.\n> \n> I think it makes sense that these are all distinct if you're using\n> coccinelle to do stylistic transformations between them (e.g., enforcing\n> curly braces even around one-liners).\n> \n> I wonder if there is a way to \"relax\" a pattern where these semantically\n> equivalent cases can all be covered automatically. I don't know enough\n> about the tool to say.\n\nHmm. They seem to call these \"standard isomorphisms\":\n\n  http://coccinelle.lip6.fr/standard.iso.html\n\nbut I'm not sure of the correct way to use them (e.g., if we want to\napply them for matching but not actually transform the code, though I am\nnot actually opposed to transforming the code, too).\n\n-Peff\n"},{"id":"355929","messageId":"CACsJy8DW6MP-a8u8KgB0ueO8d9eWmoZwF6c0Z5i+Psy980XcHg@mail.gmail.com","threadId":"49152","inReplyTo":"20180817173308.GA9111@sigill.intra.peff.net","subject":"Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-17T18:29:19Z","receivedAt":"2018-08-17T18:29:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Aug 17, 2018 at 7:33 PM Jeff King <peff@peff.net> wrote:\n>\n> On Fri, Aug 17, 2018 at 10:07:36AM -0700, Junio C Hamano wrote:\n>\n> > Junio C Hamano <gitster@pobox.com> writes:\n> >\n> > > It is a bit sad that\n> > >\n> > >     - if (E)\n> > >       FREE_AND_NULL(E);\n> > >\n> > > is not sufficient to catch it.  Shouldn't we be doing the same for\n> > > regular free(E) as well?  IOW, like the attached patch.\n> > > ...\n> >\n> > And revised even more to also spell \"E\" as \"E != NULL\" (and \"!E\" as\n> > \"E == NULL\"), which seems to make a difference, which is even more\n> > sad.  I do not want to wonder if I have to also add \"NULL == E\" and\n> > other variants, so I'll stop here.\n>\n> I think it makes sense that these are all distinct if you're using\n> coccinelle to do stylistic transformations between them (e.g., enforcing\n> curly braces even around one-liners).\n\nGoogling a bit shows a kernel patch [1]. Assuming that it works (I\ndidn't check if it made it to linux.git) it would simplify our rules a\nbit.\n\n[1] https://patchwork.kernel.org/patch/5167641/\n-- \nDuy\n"}]}