Re: [PATCH v2 01/10] hashmap: add usage documentation explaining hashmap_free[_entries]()

From: Jeff King <peff@peff.net>
Date: Oct 30, 2020, 12:50 UTC
Message-ID: <20201030125059.GA3277724@coredump.intra.peff.net>
In-Reply-To: <af6b6fcb460be900d3fffeb743a42f3f87ce6b7f.1602549650.git.gitgitgadget@gmail.com>

On Tue, Oct 13, 2020 at 12:40:41AM +0000, Elijah Newren via GitGitGadget wrote:

Show 8 quoted lines

> The existence of hashmap_free() and hashmap_free_entries() confused me,
> and the docs weren't clear enough.  We are dealing with a map table,
> entries in that table, and possibly also things each of those entries
> point to.  I had to consult other source code examples and the
> implementation.  Add a brief note to clarify the differences.  This will
> become even more important once we introduce a new
> hashmap_partial_clear() function which will add the question of whether
> the table itself has been freed.

This is a definite improvement, and I don't see any inaccuracies in the descriptions. I do think some re-naming would help in the long run, though. E.g.:

  - hashmap_clear() - remove all entries and de-allocate any
    hashmap-specific data, but be ready for reuse

  - hashmap_clear_and_free() - ditto, but free the entries themselves

  - hashmap_partial_clear() - remove all entries but don't deallocate
    table

  - hashmap_partial_clear_and_free() - ditto, but free the entries

So always call it "clear", but allow options in two dimensions (partial or not, free entries or not).

Those could be parameters to a single function, but I think it gets a little ugly because "and_free" requires passing in the type of the entries in order to find the pointers.

The "not" cases are implied in the names, but hashmap_clear_full() would be OK with me, too.

But I think in the current scheme that "free" is somewhat overloaded, and if we end with a "clear" and a "free" that seems confusing to me.

-Peff