From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 28 Aug 2019 09:03:56 GMT
Subject: Re: [PATCH 04/11] hashmap_entry: detect improper initialization
Message-ID: <8beefab6-b7ef-28d3-41d6-c5a5c042ae9a@gmail.com>
In-Reply-To: <20190827094923.6qhwqosiucsi43td@whir>

Hi Eric

On 27/08/2019 10:49, Eric Wong wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> 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.

I'm still a bit confused as the changed initializations already used 
hashmap_entry_init() and so presumably were already initializing 
hashmap_entry.next correctly. Is there a way to get 'make coccicheck' 
detect incorrect initializations, this renaming wont prevent bad code 
being added in the future.

Best Wishes

Phillip

> (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 :)
>