Re: [PATCH 03/16] object_array: factor out slopbuf-freeing logic
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.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/16] object_array: factor out slopbuf-freeing logic
On 10/08/2014 09:36 AM, Jeff King wrote:
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
mhag...@alum.mit.edu
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/16] object_array: factor out slopbuf-freeing logic
On Wed, Oct 08, 2014 at 10:40:03AM +0200, Michael Haggerty wrote: 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. :( 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/16] object_array: factor out slopbuf-freeing logic
On 10/03/2014 10:22 PM, Jeff King wrote:
This is not a lot of code, but it's a logical construct that
should not need to be repeated (and we are about to add a
third repetition).
Signed-off-by: Jeff King p...@peff.net
---
object.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/object.c b/object.c
index ca9d790..14238dc 100644
--- a/object.c
+++ b/object.c
@@ -355,6 +355,12 @@ void add_object_array_with_context(struct object *obj,
const char *name, struct
add_object_array_with_mode_context(obj, name, array,
S_IFINVALID, context);
}
+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?
[...]
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
