Re: [PATCH v4 00/13] object store: alloc
On Wed, 9 May 2018 17:40:11 -0700 Stefan Beller wrote: > 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.
Re: [PATCH v4 00/13] object store: alloc
On Thu, May 10, 2018 at 10:16 AM, Jonathan Tan wrote: > On Wed, 9 May 2018 17:40:11 -0700 > Stefan Beller wrote: > >> 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): Who doesn't love some bikeshedding in late spring? > > - I would call them release_commit() and release_tag(), to match >strbuf_release(). Why not commit_release and tag_release to also have the same order of words as in 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. We still call out to free_tree_buffer? Not sure I understand.
Re: [PATCH v4 00/13] object store: alloc
On Thu, 10 May 2018 10:32:09 -0700 Stefan Beller wrote: > > - I would call them release_commit() and release_tag(), to match > >strbuf_release(). > > Why not commit_release and tag_release to also have the same order > of words as in strbuf_release ? At this point in the discussion, either is fine. > > - 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. > > We still call out to free_tree_buffer? Not sure I understand. I meant that since we call out to free_tree_buffer (as you said), this shows that the code knows the internal details of a tree object (in that it has a buffer, and that needs to be freed, and that is the only thing that needs to be freed), so maybe the code should operate on the internal details of commits and tags as well. But again, this is a minor point.
Re: [PATCH v4 00/13] object store: alloc
On Thu, May 10, 2018 at 1:56 PM, Jonathan Tan wrote: > On Thu, 10 May 2018 10:32:09 -0700 > Stefan Beller wrote: > >> > - I would call them release_commit() and release_tag(), to match >> >strbuf_release(). >> >> Why not commit_release and tag_release to also have the same order >> of words as in strbuf_release ? > > At this point in the discussion, either is fine. ok, please express your opinion in form of a patch. ;) I do not plan on resending this series unless someone comments something that they themselves do not consider bikesheddy. Stefan