Re: [PATCH 03/16] object_array: factor out slopbuf-freeing logic

From: Jeff King <peff@peff.net>
Date: Oct 8, 2014, 08:55 UTC
Message-ID: <20141008085506.GA16314@peff.net>
In-Reply-To: <5434F863.5080303@alum.mit.edu>

On Wed, Oct 08, 2014 at 10:40:03AM +0200, Michael Haggerty wrote:

Show 8 quoted lines

> > The intent of this function is freeing memory, not clearing it for sane
> > reuse.  I think I'd be more in favor of a comment clarifying that. It is
> > a static function used only internally by the object-array code.
> 
> I guess the name reminded me of strbuf_release(), which returns the
> strbuf to its newly-initialized state (contrary to what api-strbuf.txt
> says, I just noticed). You're right that your function does no such
> thing, so it is self-consistent for it not to set ent->name to NULL.

Yeah, I had the same thought while writing it (and ended up with the same analysis you do below).

> Functions *_clear() and clear_*():

I think these ones very clearly are about reinitializing to empty (and it looks like we follow that rule, which is good).

If we were designing it now, I think strbuf_release() should probably be called strbuf_clear(). Or maybe that would be too confusing, as it might imply it is the same thing as strbuf_reset(). Yeesh. Naming is hard.

> Functions *_free() and free_*():
> 
> * Almost all of these free their arguments plus anything that their
> arguments point at.

Yes, that's the rule I think we try to follow.

> * Confusingly, free_ref_list() and free_pathspec() don't free their
> arguments, but rather only the things that their arguments points at.
> (Perhaps they should be renamed.)

Yeah, I would almost say free_pathspec should be called clear_pathspec. Except it _only_ NULLs the array. It leaves "nr" set, which means that anybody looking at it will still dereference a bogus pointer (but at least it's NULL and not freed memory!).

The free_ref_list() function is on my todo list to get rid of as part of the for-each-ref/branch/tag merger I'd like to do. But somehow that keeps slipping further down my todo list rather than actually getting finished. :(

Show 5 quoted lines

> So while three out of four *_release() functions completely reinitialize
> their arguments, there is one that doesn't. And I couldn't find enough
> other functions that just free referenced memory without reinitializing
> their whole argument to establish a naming pattern. So I guess your
> function name is OK too.

I'm open to suggestions for totally new names for this concept (free associated memory, do not reinitialize, but do not free the passed pointer). But in the absence of one, I think release() is the least-bad.

-Peff