Re: [PATCH v2 01/10] hashmap: add usage documentation explaining hashmap_free[_entries]()
- From
Elijah Newren <newren@gmail.com>
- Date
- Oct 30, 2020, 19:55 UTC
- Message-ID
- <CABPp-BGdM0tkQgqRQzzpTOyKPoQVLsppK_EUcthAO8LbtzcTNA@mail.gmail.com>
- In-Reply-To
- <20201030125059.GA3277724@coredump.intra.peff.net>
On Fri, Oct 30, 2020 at 5:51 AM Jeff King <peff@peff.net> wrote:
Show 38 quoted lines
> > On Tue, Oct 13, 2020 at 12:40:41AM +0000, Elijah Newren via GitGitGadget wrote: > > > 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.
Hmm...there are quite a few calls to hashmap_free() and hashmap_free_entries() throughout the codebase. I'm wondering if I should make switching these over to your new naming suggestions a separate follow-on series from this one, so that if there are any conflicts with other series it doesn't need to hold these first 10 patches up.
If I do that, I could also add a patch to convert several callers of hashmap_init() to use the new HASHMAP_INIT() macro, and another patch to convert shortlog to using my strset instead of its own.