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, 17:01 UTC
Message-ID: <xmqqshunf6lm.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1608021013010.79248@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 22 quoted lines

> Hi Junio,
>
> On Mon, 1 Aug 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > It would be a serious bug if hashmap_entry_init() played games with
>> > references, given its signature (that this function does not have any
>> > access to the hashmap structure, only to the entry itself):
>> >
>> > 	void hashmap_entry_init(void *entry, unsigned int hash)
>> 
>> I do not think we are on the same page.  The "reference to other
>> resource" I wondered was inside the hashmap_entry structure, IOW,
>> "the entry itself".
>
> Oh, I see now.
>
>> Which is declared to be opaque to the API users,
>
> Actually, not really. We cannot do that in C: we need to define the struct
> in hashmap.h so that its size is known to the users.

What I meant was what Documentation/technical/api-hashmap.txt said. I know that we cannot embed a true "opaque" structure in C just as you do.

Show 5 quoted lines

> That is the reason, I guess, why we have the documentation in
> Documentation/technical/api-hashmap.txt: it would have to talk about your
> hypothetical hashmap_entry_clear() (which would better be named
> *_release() BTW, unless I misunderstood what you want a hypothetical
> future version of that function to do).

I think there is no misunderstanding on your part. I am following the conclusion (as I recall) of a discussion on the list not in so distant past about X_free(x) that frees the resource an instance of "struct X" holds and also the instance itself, and X_clear(x) that only frees the resources without freeing the instance (the latter of which allows x to be on stack, you do X_init(&x) and conclude with X_clear(&x)). The name X_clear() is more in line with existing API functions like string_list_clear(), argv_array_clear(). An oddball is strbuf_release(), which I think made you make the above comment.

Show 6 quoted lines

>> I have a slight preference to avoid the lazy "void *", but that is
>> an unrelated tangent.
>
> 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.

>> I am saying that an uneven over-enginnering is bad.
>
> Hmm. I guess that the _init() function could be replaced by an _INIT macro
> a la STRBUF_INIT. Not sure it is really worth the effort, though.

I do not think so, as X_init() and X_INIT() from the point of view of the API user would not make much difference; as long as the documentation does not say it is safe to make it go out of scope without "deinitialize" it, the reader will be left wondering.