From: Jeff Hostetler <git@jeffhostetler.com>
Date: Tue, 02 Jan 2018 18:13:30 GMT
Subject: Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly
Message-ID: <90641fbf-64dd-afda-ef66-857edc6684cf@jeffhostetler.com>
In-Reply-To: <20171222171635.GA230194@google.com>



On 12/22/2017 12:16 PM, Brandon Williams wrote:
> On 12/22, Johannes Schindelin wrote:
[...]
>> 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.
>>   
>> -#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 } }

Alternatively, could we define the macro to an expression
that would cause a compiler error?  Then any new code written
would fail to compile.  And we document the issue in a comment
above the macro so no one changes the macro to "fix" it.

(I suggest this as opposed to simply removing the macro
to prevent someone from re-adding it later, since it is the
standard pattern.)

Just a thought,
Jeff