Re: [[PATCH v2] 1/4] patch-ids: stop using a hand-rolled hashmap implementation

[Junio C Hamano <gitster@pobox.com>]

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

> 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.

> 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.

>> 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.