Re: [PATCH v2 1/2] git-compat-util: add a FREEZ() wrapper around free(ptr); ptr = NULL
On Thu, Jun 15, 2017 at 6:48 PM, Junio C Hamano wrote:
> Jeff King writes:
>
>> On Sat, Jun 10, 2017 at 03:21:43AM +, Eric Wong wrote:
>>
>>> > So make Jonathan's freez_impl a public API and rename it to
>>> > free_and_null(), perhaps?
>>>
>>> Perhaps... I think it needs to take "void *" to avoid warnings:
>>>
>>> static inline void free_and_null(void *ptrptr)
>>> {
>>> void **tmp = ptrptr;
>>>
>>> free(*tmp);
>>> *tmp = NULL;
>>> }
>>
>> That unfortunately makes it very easy to get it wrong in the callers.
>> Both:
>>
>> free_and_null(&p);
>>
>> and
>>
>> free_and_null(p);
>>
>> would be accepted by the compiler, but one of them causes undefined
>> behavior.
>>
>> Unfortunately using "void **" in the declaration doesn't work, because
>> C's implicit casting rules don't apply to pointer-to-pointer types.
>
> All true.
>
> I still think the macro FREEZ() is too confusing a name to live;
> perhaps we can take Ævar's patch with s/FREEZ/FREE_AND_NULL/ and be
> done with it? By spelling it in all caps, readers will know that
> there is something special going on in that macro, and Eric's
> "forcing the readers to type & in front to let them be aware that
> the ptr variable is being manipulated" may become less necessary.
I'll change it to FREE_AND_NULL and submit my patch as-is, my reading
of the rest of this thread is that making it a function instead of a
macro would be interesting, but has its own caveats that are likely
better considered as part of its own series, whereas this just changes
existing code to its macro-expanded functional equivalent.
Re: [PATCH v2 1/2] git-compat-util: add a FREEZ() wrapper around free(ptr); ptr = NULL
Jeff King writes:
> On Sat, Jun 10, 2017 at 03:21:43AM +, Eric Wong wrote:
>
>> > So make Jonathan's freez_impl a public API and rename it to
>> > free_and_null(), perhaps?
>>
>> Perhaps... I think it needs to take "void *" to avoid warnings:
>>
>> static inline void free_and_null(void *ptrptr)
>> {
>> void **tmp = ptrptr;
>>
>> free(*tmp);
>> *tmp = NULL;
>> }
>
> That unfortunately makes it very easy to get it wrong in the callers.
> Both:
>
> free_and_null(&p);
>
> and
>
> free_and_null(p);
>
> would be accepted by the compiler, but one of them causes undefined
> behavior.
>
> Unfortunately using "void **" in the declaration doesn't work, because
> C's implicit casting rules don't apply to pointer-to-pointer types.
All true.
I still think the macro FREEZ() is too confusing a name to live;
perhaps we can take Ævar's patch with s/FREEZ/FREE_AND_NULL/ and be
done with it? By spelling it in all caps, readers will know that
there is something special going on in that macro, and Eric's
"forcing the readers to type & in front to let them be aware that
the ptr variable is being manipulated" may become less necessary.
Re: [PATCH v2 1/2] git-compat-util: add a FREEZ() wrapper around free(ptr); ptr = NULL
On Sat, Jun 10, 2017 at 03:21:43AM +, Eric Wong wrote:
> > So make Jonathan's freez_impl a public API and rename it to
> > free_and_null(), perhaps?
>
> Perhaps... I think it needs to take "void *" to avoid warnings:
>
> static inline void free_and_null(void *ptrptr)
> {
> void **tmp = ptrptr;
>
> free(*tmp);
> *tmp = NULL;
> }
That unfortunately makes it very easy to get it wrong in the callers.
Both:
free_and_null(&p);
and
free_and_null(p);
would be accepted by the compiler, but one of them causes undefined
behavior.
Unfortunately using "void **" in the declaration doesn't work, because
C's implicit casting rules don't apply to pointer-to-pointer types.
-Peff
Re: [PATCH v2 1/2] git-compat-util: add a FREEZ() wrapper around free(ptr); ptr = NULL
On Jun 09 2017, Jonathan Nieder wrote: > That way side-effectful callers like FREEZ(func() ? a : b) would > work. Except that you cannot take the address of a non-lvalue. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different."
Re: [PATCH v2 1/2] git-compat-util: add a FREEZ() wrapper around free(ptr); ptr = NULL
Junio C Hamano wrote:
> Eric Wong writes:
> > 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.
>
> Sounds sensible.
>
> So make Jonathan's freez_impl a public API and rename it to
> free_and_null(), perhaps?
Perhaps... I think it needs to take "void *" to avoid warnings:
static inline void free_and_null(void *ptrptr)
{
void **tmp = ptrptr;
free(*tmp);
*tmp = NULL;
}
...At least I needed to do that for "mog_free_and_null" in
cmogstored:
https://bogomips.org/cmogstored.git/plain/alloc.c?h=v1.6.0
But heck, maybe that's covering up for something else I got
wrong in cmogstored *shrug*I don't know all of C.
Re: [PATCH v2 1/2] git-compat-util: add a FREEZ() wrapper around free(ptr); ptr = NULL
Eric Wong writes: > Jonathan Nieder 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"). Both match my thoughts exactly ;-) > ... > 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. Sounds sensible. So make Jonathan's freez_impl a public API and rename it to free_and_null(), perhaps?
Re: [PATCH v2 1/2] git-compat-util: add a FREEZ() wrapper around free(ptr); ptr = NULL
Jonathan Nieder 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.
> > 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.
> 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.
Re: [PATCH v2 1/2] git-compat-util: add a FREEZ() wrapper around free(ptr); ptr = NULL
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.
>
> (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
> ---
> 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
