Hi,
Ævar Arnfjörð Bjarmason wrote:
> Add a FREEZ() wrapper marco for the common pattern of freeing a
> pointer and assigning NULL to it right afterwards.
I'm conflicted. On one hand it makes code more concise and makes it
easier for people to remember to assign NULL after freeing a variable.
On the other hand it makes git more of a custom dialect of C, which
may make the code harder to read and hack on for new contributors.
My feeling is that the costs outweigh the benefits, but I haven't
thought it through thoroughly.
> The implementation is similar to the (currently unused) XDL_PTRFREE
> macro in xdiff/xmacros.h added in commit 3443546f6e ("Use a *real*
> built-in diff generator", 2006-03-24). The only difference is that
> free() is called unconditionally, see [1].
>
> 1. <alpine.DEB.2.20.1608301948310.129229@virtualbox>
> (http://public-inbox.org/git/alpine.DEB.2.20.1608301948310.129229@virtualbox/)
Nit: it's redundant to state the message-id twice.
Can this commit message include a summary of that conversation so
someone trying to understand this patch with "git log" does not have
to open a browser and re-read the thread? Is that thread being cited
for the point that git uses 'free(NULL)' without worrying? I think
that's okay to say without citation --- there are lots of examples
throughout the git codebase.
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> git-compat-util.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 4b7dcf21ad..ba2d0c8c80 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -805,6 +805,12 @@ extern int xmkstemp_mode(char *template, int mode);
> extern char *xgetcwd(void);
> extern FILE *fopen_for_writing(const char *path);
>
> +/*
> + * FREEZ(ptr) is like free(ptr) followed by ptr = NULL. Note that ptr
> + * is used twice, so don't pass e.g. ptr++.
> + */
> +#define FREEZ(p) do { free(p); (p) = NULL; } while (0)
> +
> #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
> #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), (alloc)))
Golfing: it's possible to do this without ptr being used twice by
introducing a helper function:
static inline void freez_impl(void **p) {
free(*p);
*p = NULL;
}
#define FREEZ(p) freez_impl(&(p))
That way side-effectful callers like FREEZ(func() ? a : b) would
work.
Another request: if rolling out another version of this series, could
it go in a new thread? The cover letter can link to the old version
for context. Each time I see a new reply to the repository object
series my heart leaps for a moment, in the hope that we're getting
closer to have a repository object in git.
I kind of wish that 'free' returned NULL so that callers could do
p = free(p);
without requiring a custom helper. We could introduce a free_wrapper
that works that way but that is probably not worth it, either.
Thanks for an interesting and pleasant read.
Sincerely,
Jonathan