Glen Choo <chooglen@xxxxxxxxxx> writes:
> I suppose the question of whether or not to free() in the 'destructor'
> depends on whether we expect the struct to be reusable? I don't expect
> that "struct remote_state" needs to be reused, so free()-ing it is ok to
> me.
>
> The API is not _that_ odd though ;) As you noted, my initial use of
> FREE_AND_NULL() is for consistency reasons with the rest of
> repo_clear(), which looks like this:
>
> if (repo->config) {
> git_configset_clear(repo->config);
> FREE_AND_NULL(repo->config);
So git_configset_clear() does clear but does not free.
> }
>
> if (repo->submodule_cache) {
> submodule_cache_free(repo->submodule_cache);
submodule_cache_free() does (probably clear and) free.
> repo->submodule_cache = NULL;
> }
>
> if (repo->index) {
> discard_index(repo->index);
discard_index() does not free.
> if (repo->index != &the_index)
> FREE_AND_NULL(repo->index);
> }
>
> if (repo->promisor_remote_config) {
> promisor_remote_clear(repo->promisor_remote_config);
promisor_remote_clear() does not free.
> FREE_AND_NULL(repo->promisor_remote_config);
> }
>
> if (repo->remote_state) {
> remote_state_clear(repo->remote_state);
> - FREE_AND_NULL(repo->remote_state);
> + repo->remote_state = NULL;
> }
>
> promisor_remote_clear(), discard_index(), and git_configset_clear()
> don't free() the struct, so it makes sense for them to use
> FREE_AND_NULL(). AFAICT, these structs are meant to be reused, so it
> makes sense that we "clear" it without freeing the struct pointer
> itself.
>
> On the other hand, submodule_cache_free() _does_ free() the struct, and
> so we just use "= NULL". I noticed that this uses the verb "free", and
> not "clear".
>
> So now that remote_state_clear() *does* free() the struct, it is
> perfectly fine to use "= NULL" here as well, though it uses the verb
> "clear".
>
> I'm not sure if we have a style around clear/free. Feel free to ignore
> if there isn't one.
It does bother me. Changing _clear() that did not free the
container resource to free it, without changing the name to free or
release or whatever, smells like leaving a source of confusion for
future developers.
