On Mon, Nov 11, 2024 at 04:22:26PM -0500, Simon Marchi wrote: > > The fix makes sense. I wondered if this had been broken for a long time, > > and if so, how we managed not to notice it. But it looks like it is a > > recent problem, via 7f795a1715 (builtin/difftool: plug several trivial > > memory leaks, 2024-09-26). > > Are there tests for this specific scenario (no diff between the two > versions)? Presumably not, or they'd have been segfaulting. ;) So it may be worth adding some to t7800. > > I'm not sure if zero-initialization is being a little too familiar with > > the hashmap internals, though. > > Up to you. In other C projects I worked on, it was typical that > zero-ing an object would get it in a valid initial empty state, properly > handled by the destruction functions. This way, a big struct containing > other objects could be initialized simply by zero-ing it, without having > to initialize each component explicitly. We often follow that rule in git.git, too, but have been increasingly moving to macro initializers. They make it easier if we ever need a non-zero state as an invariant (e.g., STRBUF_INIT always points to a dummy string). In this case, it does look like HASHMAP_INIT sets do_count_items. It's not a bug in your program since we still hashmap_init() over the zero'd state, but it does feel a bit weird to me. > Please let me know if you want a v2 or if you are just going to merge an > updated version of this patch. That's up to the maintainer. IMHO it's worth using HASHMAP_INIT, though, and perhaps adding a test. -Peff
