Re: [[PATCH v2] 1/4] patch-ids: stop using a hand-rolled hashmap implementation
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 2, 2016, 18:04 UTC
- Message-ID
- <xmqqtwf3dp4a.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <xmqqshunf6lm.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >> Oh, we are already safely in Unrelated Tangent Land for a while, I would >> think. Nothing of what we are discussing in this thread has anything to do >> with Kevin's patch series,... > > Oh, no question about that. Go back to my review, to which your > message I am responding to is a reply to. What you wrote are all > about things after "This is a tangent, but this made me wonder if it > is safe to simply free(3) the result...", which pointed out rough > points in the hashmap API from the API user's point of view and > suggested a few possible improvement opportunities.
In other words, I'd be happy with a patch like this, outside the scope of and independent from this series.
When the hashmap_entry structure does acquire references to external resources (which I wouldn't judge the likelihood of), this paragraph will become stale, but that is exactly the point at which _clear() function needs to be added to the API and described here, replacing this paragraph.
I do not mind having an empty _clear() before that happens, but I do not think it adds much safety. A disciplined user of the API may call that empty _clear() to make her code future-proof, but we know there are undisciplined developers and reviewers and there will be codepaths that call _init() without calling the empty _clear(), and we won't notice it. Whoever is adding the real need for _clear() must audit the codebase at that point _anyway_, and that is why I think having an empty _clear() before would not buy us much.
Documentation/technical/api-hashmap.txt | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/Documentation/technical/api-hashmap.txt b/Documentation/technical/api-hashmap.txt index ad7a5bd..1dcec3d 100644 --- a/Documentation/technical/api-hashmap.txt +++ b/Documentation/technical/api-hashmap.txt @@ -104,6 +104,11 @@ If `free_entries` is true, each hashmap_entry in the map is freed as well `entry` points to the entry to initialize. + `hash` is the hash code of the entry. ++ +The hashmap_entry structure does not hold references to external resources, +and it is safe to just discard it once you are done with it (i.e. if +your structure was allocated with xmalloc(), you can just free(3) it, +and if it is on stack, you can just let it go out of scope). `void *hashmap_get(const struct hashmap *map, const void *key, const void *keydata)`::