[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH 04/11] hashmap_entry: detect improper initialization
From:       Eric Wong <e () 80x24 ! org>
Date:       2019-08-30 19:52:40
Message-ID: 20190830195240.ephg6r6zjbrkabvp () dcvr
[Download RAW message or body]

Phillip Wood <phillip.wood123@gmail.com> wrote:
> 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.

Yeah I forgot we had coccicheck :x

I think this patch to rename the field can be dropped entirely.
I changed some usages of hashmap_entry_init to avoid reading the
.hash field entirely, since the result of memhash() could be
stored locally for multiple uses of hashmap_entry_init.
[prev in list] [next in list] [prev in thread] [next in thread]