Re: [PATCH v4 00/13] object store: alloc
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- May 10, 2018, 17:16 UTC
- Message-ID
- <20180510101615.931eb0d219eac4c84cf8140d@google.com>
- In-Reply-To
- <20180510004024.93974-1-sbeller@google.com>
On Wed, 9 May 2018 17:40:11 -0700 Stefan Beller <sbeller@google.com> wrote:
Show 9 quoted lines
> if (obj->type == OBJ_TREE) > - release_tree_node((struct tree*)obj); > + free_tree_buffer((struct tree*)obj); > else if (obj->type == OBJ_COMMIT) > - release_commit_node((struct commit*)obj); > + release_commit_memory((struct commit*)obj); > else if (obj->type == OBJ_TAG) > - release_tag_node((struct tag*)obj); > + free_tag_buffer((struct tag*)obj);
This might seem a bit bikesheddy, but I wouldn't call it free_tag_buffer(), since what's being freed is not the buffer of the object itself, but just a string. If you want such a function, I would just call it release_tag_memory() to match release_commit_memory().
Other than that, all the patches look fine to me.
Some optional comments (this is almost certainly bikeshedding):
- I would call them release_commit() and release_tag(), to match strbuf_release(). - It might be better to just inline the handling of releasing commit and tag memory. This code already knows that, for a tree, it needs to free its buffer and only its buffer, so it is not much of a stretch to think that it similarly knows the details of commit and tag objects too.