Re: [PATCH] refactor various if (x) FREE_AND_NULL(x) to just FREE_AND_NULL(x)

From: Jeff King <peff@peff.net>
Date: Aug 17, 2018, 17:33 UTC
Message-ID: <20180817173308.GA9111@sigill.intra.peff.net>
In-Reply-To: <xmqqlg94c46f.fsf@gitster-ct.c.googlers.com>

On Fri, Aug 17, 2018 at 10:07:36AM -0700, Junio C Hamano wrote:

Show 15 quoted lines

> Junio C Hamano <gitster@pobox.com> 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