Re: [PATCH 04/11] hashmap_entry: detect improper initialization

From: Eric Wong <e@80x24.org>
Date: Aug 27, 2019, 09:49 UTC
Message-ID: <20190827094923.6qhwqosiucsi43td@whir>
In-Reply-To: <nycvar.QRO.7.76.6.1908271108410.46@tvgsbejvaqbjf.bet>

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

Show 13 quoted lines

> Hi Eric,
> 
> On Mon, 26 Aug 2019, Eric Wong wrote:
> 
> > By renaming the "hash" field to "_hash", it's easy to spot
> > improper initialization of hashmap_entry structs which
> > can leave "hashmap_entry.next" uninitialized.
> 
> Would you mind elaborating a bit? This explanation does not enlighten
> me, sadly, all I see is that it makes it (slightly) harder for me to
> maintain Git for Windows' patches on top of `pu`, as the FSCache patches
> access that field directly (so even if they rebase cleanly, the build
> breaks).

I renamed it to intentionally break my build.

That way I could easily spot if there were any other improper initializations of the .hash field. It's fine to revert, actually, it could be more of a "showing my work" patch.

(AFAIK, it's a pretty common practice, but maybe not here :x)

I've also pondered adding a "hashmap_entry_hash(const struct hashmap_entry *)" accessor method for reading the field value (but not setting it), but it's a bit verbose...

I'm also wondering where/if hashmap offers a real benefit over khash nowadays; the latter ought to have better locality. Would like benchmark at some point in the future; but safety fixes first :)