Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)
On Fri, Aug 17, 2018 at 7:33 PM Jeff King wrote: > > On Fri, Aug 17, 2018 at 10:07:36AM -0700, Junio C Hamano wrote: > > > Junio C Hamano writes: > > > > > It is a bit sad that > > > > > > - if (E) > > > FREE_AND_NULL(E); > > > > > > is not sufficient to catch it. Shouldn't we be doing the same for > > > regular free(E) as well? IOW, like the attached patch. > > > ... > > > > And revised even more to also spell "E" as "E != NULL" (and "!E" as > > "E == NULL"), which seems to make a difference, which is even more > > sad. I do not want to wonder if I have to also add "NULL == E" and > > other variants, so I'll stop here. > > I think it makes sense that these are all distinct if you're using > coccinelle to do stylistic transformations between them (e.g., enforcing > curly braces even around one-liners). Googling a bit shows a kernel patch [1]. Assuming that it works (I didn't check if it made it to linux.git) it would simplify our rules a bit. [1] https://patchwork.kernel.org/patch/5167641/ -- Duy
Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)
On Fri, Aug 17, 2018 at 01:33:08PM -0400, Jeff King wrote: > > And revised even more to also spell "E" as "E != NULL" (and "!E" as > > "E == NULL"), which seems to make a difference, which is even more > > sad. I do not want to wonder if I have to also add "NULL == E" and > > other variants, so I'll stop here. > > I think it makes sense that these are all distinct if you're using > coccinelle to do stylistic transformations between them (e.g., enforcing > curly braces even around one-liners). > > I wonder if there is a way to "relax" a pattern where these semantically > equivalent cases can all be covered automatically. I don't know enough > about the tool to say. Hmm. They seem to call these "standard isomorphisms": http://coccinelle.lip6.fr/standard.iso.html but I'm not sure of the correct way to use them (e.g., if we want to apply them for matching but not actually transform the code, though I am not actually opposed to transforming the code, too). -Peff
Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)
On Fri, Aug 17, 2018 at 01:39:51PM -0400, Jeff King wrote: > > I wonder if there is a way to "relax" a pattern where these semantically > > equivalent cases can all be covered automatically. I don't know enough > > about the tool to say. > > Hmm. They seem to call these "standard isomorphisms": > > http://coccinelle.lip6.fr/standard.iso.html > > but I'm not sure of the correct way to use them (e.g., if we want to > apply them for matching but not actually transform the code, though I am > not actually opposed to transforming the code, too). Hmph, I should really pause before hitting 'send'. Last message, I promise. :) I do not see an option to include a list an arbitrary set of isomorphisms, but the standard.iso list should be used by default. I wonder if you simply need to write your case in the normalized version they use there (which I think is "X == NULL"), and the others would be taken care of. -Peff
Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)
On Fri, Aug 17, 2018 at 10:07:36AM -0700, Junio C Hamano wrote: > Junio C Hamano writes: > > > It is a bit sad that > > > > - if (E) > > FREE_AND_NULL(E); > > > > is not sufficient to catch it. Shouldn't we be doing the same for > > regular free(E) as well? IOW, like the attached patch. > > ... > > And revised even more to also spell "E" as "E != NULL" (and "!E" as > "E == NULL"), which seems to make a difference, which is even more > sad. I do not want to wonder if I have to also add "NULL == E" and > other variants, so I'll stop here. I think it makes sense that these are all distinct if you're using coccinelle to do stylistic transformations between them (e.g., enforcing curly braces even around one-liners). I wonder if there is a way to "relax" a pattern where these semantically equivalent cases can all be covered automatically. I don't know enough about the tool to say. I guess one way to do it would be to normalize the style in one rule (e.g., always "!E" instead of "E == NULL"), and then you only have to write the FREE_AND_NULL rule for the normalized form. For a single case like this, the end result is about the same number of rules, but in the long term it saves us work when we have a similar transformation. -Peff
Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)
Junio C Hamano writes:
> It is a bit sad that
>
> - if (E)
> FREE_AND_NULL(E);
>
> is not sufficient to catch it. Shouldn't we be doing the same for
> regular free(E) as well? IOW, like the attached patch.
> ...
And revised even more to also spell "E" as "E != NULL" (and "!E" as
"E == NULL"), which seems to make a difference, which is even more
sad. I do not want to wonder if I have to also add "NULL == E" and
other variants, so I'll stop here.
contrib/coccinelle/free.cocci | 60 +++
1 file changed, 60 insertions(+)
diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
index 4490069df9..29ca98796f 100644
--- a/contrib/coccinelle/free.cocci
+++ b/contrib/coccinelle/free.cocci
@@ -16,3 +16,63 @@ expression E;
- free(E);
+ FREE_AND_NULL(E);
- E = NULL;
+
+@@
+expression E;
+@@
+- if (E)
+ FREE_AND_NULL(E);
+
+@@
+expression E;
+@@
+- if (E != NULL)
+ free(E);
+
+@@
+expression E;
+@@
+- if (E == NULL)
+ free(E);
+
+@@
+expression E;
+@@
+- if (E != NULL)
+ FREE_AND_NULL(E);
+
+@@
+expression E;
+@@
+- if (E) { free(E); }
++ free(E);
+
+@@
+expression E;
+@@
+- if (!E) { free(E); }
++ free(E);
+
+@@
+expression E;
+@@
+- if (E) { FREE_AND_NULL(E); }
++ FREE_AND_NULL(E);
+
+@@
+expression E;
+@@
+- if (E != NULL) { free(E); }
++ free(E);
+
+@@
+expression E;
+@@
+- if (E == NULL) { free(E); }
++ free(E);
+
+@@
+expression E;
+@@
+- if (E != NULL) { FREE_AND_NULL(E); }
++ FREE_AND_NULL(E);
Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)
Duy Nguyen writes:
> Just fyi this seems to do the trick. Although I'm nowhere good at
> coccinelle to say if we should include this (or something like it)
>
> -- 8< --
> diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
> index 4490069df9..f8e018d104 100644
> --- a/contrib/coccinelle/free.cocci
> +++ b/contrib/coccinelle/free.cocci
> @@ -16,3 +16,9 @@ expression E;
> - free(E);
> + FREE_AND_NULL(E);
> - E = NULL;
> +
> +@@
> +expression E;
> +@@
> +- if (E) { FREE_AND_NULL(E); }
> ++ FREE_AND_NULL(E);
It is a bit sad that
- if (E)
FREE_AND_NULL(E);
is not sufficient to catch it. Shouldn't we be doing the same for
regular free(E) as well? IOW, like the attached patch.
contrib/coccinelle/free.cocci | 24
1 file changed, 24 insertions(+)
diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
index 4490069df9..f748bcfe30 100644
--- a/contrib/coccinelle/free.cocci
+++ b/contrib/coccinelle/free.cocci
@@ -16,3 +16,27 @@ expression E;
- free(E);
+ FREE_AND_NULL(E);
- E = NULL;
+
+@@
+expression E;
+@@
+- if (E)
+ FREE_AND_NULL(E);
+
+@@
+expression E;
+@@
+- if (E) { free(E); }
++ free(E);
+
+@@
+expression E;
+@@
+- if (!E) { free(E); }
++ free(E);
+
+@@
+expression E;
+@@
+- if (E) { FREE_AND_NULL(E); }
++ FREE_AND_NULL(E);
Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)
On Fri, Aug 17, 2018 at 04:36:13PM +0200, Duy Nguyen wrote:
> On Fri, Aug 17, 2018 at 3:05 PM Ævar Arnfjörð Bjarmason
> wrote:
> >
> > Change the few conditional uses of FREE_AND_NULL(x) to be
> > unconditional. As noted in the standard[1] free(NULL) is perfectly
> > valid, so we might as well leave this check up to the C library.
>
> I'm not trying to make you work more on this. But out of curiosity
> would coccinelle help catch this pattern? Szeder's recent work on
> running cocci automatically would help catch all future code like this
> if we could write an spatch.
Just fyi this seems to do the trick. Although I'm nowhere good at
coccinelle to say if we should include this (or something like it)
-- 8< --
diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
index 4490069df9..f8e018d104 100644
--- a/contrib/coccinelle/free.cocci
+++ b/contrib/coccinelle/free.cocci
@@ -16,3 +16,9 @@ expression E;
- free(E);
+ FREE_AND_NULL(E);
- E = NULL;
+
+@@
+expression E;
+@@
+- if (E) { FREE_AND_NULL(E); }
++ FREE_AND_NULL(E);
-- 8< --
--
Duy
Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)
On Fri, Aug 17, 2018 at 3:05 PM Ævar Arnfjörð Bjarmason wrote: > > Change the few conditional uses of FREE_AND_NULL(x) to be > unconditional. As noted in the standard[1] free(NULL) is perfectly > valid, so we might as well leave this check up to the C library. I'm not trying to make you work more on this. But out of curiosity would coccinelle help catch this pattern? Szeder's recent work on running cocci automatically would help catch all future code like this if we could write an spatch. -- Duy
