Johannes Schindelin [off-list ref] 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".
Which is declared to be opaque to the API users, so whoever defined
that API cannot blame me for not checking its definition to see that
it only has "unsigned int hash" and no allocated memory or open file
descriptor in it that needs freeing.
By the way, the first parameter of the function being "void *" is
merely to help lazy API users, who have their own structure that
embeds the hashmap_entry as its first element, as API documentation
tells them to do, e.g.
struct foo {
struct hashmap_entry e;
... other "foo" specific fields come here ...
} foo;
and because of the lazy "void *", they do not have to do this:
hashmap_entry_init(&foo->e, ...);
which would be required if the first parameter were "struct
hashmap_entry *", but they can just do this:
hashmap_entry_init(&foo, ...);
I have a slight preference to avoid the lazy "void *", but that is
an unrelated tangent.
quoted
The fact that hashmap_entry_init() is there but there is no
corresponding hashmap_entry_clear() hints that there is nothing to
be worried about and I can see from the implementation of
hashmap_entry_init() that no extra resource is held inside, but an
API user should not have to guess. We may want to do one of the two
things:
* document that an embedded hashmap_entry does not hold any
resource that need to be released and it is safe to free the user
structure that embeds one; or
* implement hashmap_entry_clear() that currently is a no-op.
Urgh. The only reason we have hashmap_entry_init() is that we *may* want
to extend `struct hashmap_entry` at some point. That is *already*
over-engineered because that point in time seems quite unlikely to arrive,
like, ever.
I am saying that an uneven over-enginnering is bad.
To be consistent with having _init() and declaring the entry
structure to be opaque, you would either need _clear() or document
you would never need to worry about the lack of _clear().
Junio C Hamano [off-list ref] wrote:
Johannes Schindelin [off-list ref] writes:
quoted
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)
<snip>
I have a slight preference to avoid the lazy "void *", but that is
an unrelated tangent.
Me too. I noticed this while working on the http-walker
speedups and my self-rejected last patch:
https://public-inbox.org/git/20160711210243.GA1604%40whir/
Extracting list_entry from list.h (currently in next[1]) and
exposing that generically as "container_of" for use with
hashmap_* would make it safer-to-use and allow structs to belong
to multiple hashmaps.
list_entry is also an alias for container_of in the Linux
kernel, but we don't have enough code using list_* to
warrant two names for the same macro.
[1] https://public-inbox.org/git/20160711205131.1291-4-e%4080x24.org/
Hi Junio,
On Mon, 1 Aug 2016, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
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.
so whoever defined that API cannot blame me for not checking its
definition to see that it only has "unsigned int hash" and no allocated
memory or open file descriptor in it that needs freeing.
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).
And quite frankly, unless we *have* to, I would rather try to avoid
introducing that function as much as possible, as it would make using the
hashmap API even more finicky than it already is.
By the way, the first parameter of the function being "void *" is
merely to help lazy API users, who have their own structure that
embeds the hashmap_entry as its first element, as API documentation
tells them to do, e.g.
struct foo {
struct hashmap_entry e;
... other "foo" specific fields come here ...
} foo;
and because of the lazy "void *", they do not have to do this:
hashmap_entry_init(&foo->e, ...);
which would be required if the first parameter were "struct
hashmap_entry *", but they can just do this:
hashmap_entry_init(&foo, ...);
Yes, I know that. It is the common way to simulate subclassing in C, for
lack of a more compile-safe construct.
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, which is about trying to use resources more
sensibly when using the revision machinery's --cherry-pick option.
And since we are already there, I'll offer an opinion in favor of `void
*`: doing the &foo->e dance could quite possibly suggest that `e` is a
field just like any other field (and does not necessarily *need* to be the
first).
But again, this has nothing to do with the patch series we are discussing
here.
quoted
quoted
The fact that hashmap_entry_init() is there but there is no
corresponding hashmap_entry_clear() hints that there is nothing to be
worried about and I can see from the implementation of
hashmap_entry_init() that no extra resource is held inside, but an
API user should not have to guess. We may want to do one of the two
things:
* document that an embedded hashmap_entry does not hold any
resource that need to be released and it is safe to free the user
structure that embeds one; or
* implement hashmap_entry_clear() that currently is a no-op.
Urgh. The only reason we have hashmap_entry_init() is that we *may* want
to extend `struct hashmap_entry` at some point. That is *already*
over-engineered because that point in time seems quite unlikely to arrive,
like, ever.
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.
Ciao,
Dscho