Hi,
On 2022-08-22 14:30:22 -0400, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> > Per discussion in [0], here is a patch set that allows pfree() to accept
> > a NULL argument, like free() does.
>
> So the question is, is this actually a good thing to do?
>
> If we were starting in a green field, I'd be fine with defining
> pfree(NULL) as okay. But we're not, so there are a couple of big
> objections:
>
> * Code developed to this standard will be unsafe to back-patch
>
> * The sheer number of places touched will create back-patching
> hazards.
>
> I'm not very convinced that the benefits of making pfree() more
> like free() are worth those costs.
It's probably also not entirely cost free due to the added branches in place
we are certain that the pointer is non-null. That could partially be
ameliorated by moving the NULL pointer check into the callers.
If we don't want to go this route it might be worth adding a
pg_attribute_nonnull() or such to pfree().
> (FWIW, no objection to your 0001. 0004 and 0005 seem okay too;
> they don't touch enough places to create much back-patching risk.)
I like 0001, not sure I find 0004, 0005 an improvement.
Semi-related note: I've sometimes wished for a pfreep(void **p) that'd do
something like
if (*p)
{
pfree(*p);
*p = NULL;
}
so there's no dangling pointers after the pfree(), which often enoughis
important (e.g. because the code could be reached again if there's an error)
and is also helpful when debugging. The explicit form does bulk up code
sufficiently to be annoying.
Greetings,
Andres Freund