Re: [PATCH 03/11] hashmap_entry_init takes "struct hashmap_entry *"

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 2, 2019, 13:46 UTC
Message-ID: <nycvar.QRO.7.76.6.1909021542160.46@tvgsbejvaqbjf.bet>
In-Reply-To: <20190830194855.45jwupvp5uqcg46y@dcvr>

Hi Eric,

On Fri, 30 Aug 2019, Eric Wong wrote:

Show 22 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Tue, 27 Aug 2019, Derrick Stolee wrote:
> >
> > > On 8/25/2019 10:43 PM, Eric Wong wrote:
> > > > C compilers do type checking to make life easier for us.  So
> > > > rely on that and update all hashmap_entry_init callers to take
> > > > "struct hashmap_entry *" to avoid future bugs while improving
> > > > safety and readability.
> > >
> > > Overall I like this change. I'll need to keep it in mind with my
> > > sparse-checkout work that is adding more hashmap types.
> > >
> > > One _might_ think that this change is relaxing the condition on
> > > where the hashmap_entry appears within the super-struct, but
> > > the hashmap internals will still use void* and perform a cast
> > > to hashmap_entry for hash comparisons.
> >
> > I thought precisely the same.
>
> Yes, that's the goal I'm working towards as mentioned in patches
> 10 and 11 :)

Well, it is not exactly clear to me how memory management would work (I have not read patch 10 or 11 due to lack of time). At the moment, releasing a hashmap means we can simply `free()` the stored pointers. If you drop the requirement to make the `hashmap_entry` field the first field of the enclosing `struct`, that is no longer possible.

Of course, you could add an `offsetof` value to the hashmap that says what number we have to subtract from the stored pointer before calling `free()`, but that sounds quite a bit more fragile to me than the current design.

Show 5 quoted lines

> > Maybe we can get a Coccinelle rule that verifies that `struct
> > hashmap_entryh` fields are always the first ones?
>
> At this point, why?  Given the goal is to remove that requirement
> entirely.

With a Coccinelle rule, we could maintain a simple design *and* be certain that no user violates the assumption that the `hashmap_entry` field is the first one in the enclosing `struct`.

Ciao, Dscho