On 12/22, Johannes Schindelin wrote: > In Git's source code, we have this convention that quite a few data > structures can be initialized using a macro *_INIT while defining an > instance (instead of having to call a function to initialize the data > structure). You will see that idiom quite a bit, e.g. `struct strbuf buf > = STRBUF_INIT;` > > This works for oidsets, too: `struct oidset oids = OIDSET_INIT;` is > perfectly legal and you can use that data structure right away, without > having to call `oidset_init()` first. > > That pattern is violated by OIDMAP_INIT, though. The first call to > oidmap_put() or oidmap_get() will succeed, but by mistake rather than by > design: The underlying hashmap is not initialized correctly, as the > cmpfn function pointer still points to NULL, but since there are no > entries to be compared, cmpfn will not be called. Things break down, > though, as soon as there is even one entry. > > Rather than causing confusion, frustration and needless loss of time due > to pointless debugging, let's *rename* OIDMAP_INIT so that developers > who have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do > that with oidmaps. > > An alternative would be to introduce a HASHMAP_INIT(cmp_fn) and use that > in oidmap.h. However, there are *no* call sites in Git's source code > that would benefit from that macro, i.e. this would be premature > optimization (and cost a lot more time than this here trivial patch). > > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx> > --- > Published-As: https://github.com/dscho/git/releases/tag/discourage-OIDMAP_INIT-v1 > Fetch-It-Via: git fetch https://github.com/dscho/git discourage-OIDMAP_INIT-v1 > oidmap.h | 6 +++++- > oidset.h | 7 ++++++- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/oidmap.h b/oidmap.h > index 18f54cde143..1a73c392b79 100644 > --- a/oidmap.h > +++ b/oidmap.h > @@ -21,7 +21,11 @@ struct oidmap { > struct hashmap map; > }; > > -#define OIDMAP_INIT { { NULL } } > +/* > + * This macro initializes the data structure only incompletely, just enough > + * to call oidmap_get() on an empty map. Use oidmap_init() instead. > + */ > +#define OIDMAP_INIT_INCOMPLETELY { { NULL } } This is one way of approaching the problem. Couldn't we also take the approach like we have with oidset and ensure that when oidmap_get() or _put() are called that if the oidmap isn't initialized, we simply do that then? > > /* > * Initializes an oidmap structure. > diff --git a/oidset.h b/oidset.h > index f4c9e0f9c04..a11d88edc1d 100644 > --- a/oidset.h > +++ b/oidset.h > @@ -22,7 +22,12 @@ struct oidset { > struct oidmap map; > }; > > -#define OIDSET_INIT { OIDMAP_INIT } > +/* > + * It is okay to initialize the map incompletely here because oidset_insert() > + * will call oidset_init() (which will call oidmap_init()), and > + * oidset_contains() works as intended even before oidset_init() was called. > + */ > +#define OIDSET_INIT { OIDMAP_INIT_INCOMPLETELY } > > /** > * Returns true iff `set` contains `oid`. > > base-commit: 936d1b989416a95f593bf81ccae8ac62cd83f279 > -- > 2.15.1.windows.2 -- Brandon Williams
