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

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Oct 8, 2014, 08:40 UTC
Message-ID: <5434F863.5080303@alum.mit.edu>
In-Reply-To: <20141008073658.GC25250@peff.net>

On 10/08/2014 09:36 AM, Jeff King wrote:

Show 18 quoted lines

> On Tue, Oct 07, 2014 at 01:25:54PM +0200, Michael Haggerty wrote:
> 
>>> +static void object_array_release_entry(struct object_array_entry *ent)
>>> +{
>>> +	if (ent->name != object_array_slopbuf)
>>> +		free(ent->name);
>>> +}
>>> +
>>
>> Would it be a little safer to set ent->name to NULL or to
>> object_array_slopbuf after freeing the memory, to prevent accidents?
> 
> I considered that, but what about the other parts of object_array_entry?
> Should we NULL the object context pointers, too?
> 
> 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.

But maybe its name could be chosen better? Let's see if there is a consensus naming policy for functions that free resources. I grepped for short functions calling free() and visually inspected a bunch of them.

Functions *_release():

* strbuf_release(), range_set_release(), and diff_ranges_release()
completely reinitialize their arguments

* window_release() appears not to

Functions *_clear() and clear_*():

* All *_clear() functions that I looked at (e.g., argv_array_clear(),
clear_image(), credential_clear(), clear_exclude_list(),
signature_check_clear(), and clear_prio_queue()) completely reinitialize
their arguments

Functions *_free() and free_*():

* Almost all of these free their arguments plus anything that their
arguments point at.

* 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.)

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.

So forget I said anything :-)

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu