Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
> 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.
I think this problem could be avoided by using a more explicit
name, perhaps: "free_and_null"
Seeing the initial subject, I thought this series was short for
"freeze" (like "creat").
However, I admit FREEZ caught my eye because I thought it was
a way to freeze a repository, somehow :)
> My feeling is that the costs outweigh the benefits, but I haven't
> thought it through thoroughly.
<snip>
> > 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;
> }
Yes. I think it's prudent to avoid macros in case there are
side effects.
> #define FREEZ(p) freez_impl(&(p))
>
> That way side-effectful callers like FREEZ(func() ? a : b) would
> work.
I don't see the point of a macro wrapper, forcing the user to
type out the '&' should drive home the point that the pointer
gets set to NULL. I also find capitalization tiring-to-read
because all characters are the same height.
<snip>
> 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.
Sometimes I have wished similar things, too, but that means the
same identifier shows up twice in one line and camouflages the
code.