Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval
- From
Tanay Abhra <tanayabh@gmail.com>
- Date
- Jun 24, 2014, 12:21 UTC
- Message-ID
- <53A96D2D.1020102@gmail.com>
- In-Reply-To
- <xmqqwqc79q1p.fsf@gitster.dls.corp.google.com>
On 6/24/2014 4:44 AM, Junio C Hamano wrote:
Show 30 quoted lines
> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> +static int hashmap_initialized;
>> + ...
>> +static struct hashmap *get_config_cache(void)
>> +{
>> + static struct hashmap config_cache;
>> + if (!hashmap_initialized) {
>> + config_cache_init(&config_cache);
>> + hashmap_initialized = 1;
>
> I find the arrangement of these two variables somewhat dubious at
> the API design level.
>
> If you are going to keep the singleton "config_cache" as a function
> scope static, shouldn't the corresponding guard also be in the same
> scope?
>
> If you ever need to "uninitialize" to force re-read the file to the
> in-core cache, such an uninitializer will need access to not just
> the "is hashmap initialized?" boolean (which you do by having it as
> a file-scope global like this patch does) but also the thing that
> may need to be uninitialized (i.e. the hashmap that may already be
> populated), but a function scope static variable config_cache does
> not allow access from other places, so you end up calling this
> function to initialize it if necessary only to get the pointer to
> that structure in order to uninitialize it.
>
> Sounds very twisted and ugly, doesn't it?
>Hmn, You are right. but I couldn't find a way for that without making "config_cache" file-scope static.
For the config_cache_free(), would this change be enough?
+static void config_cache_free(void) +{ + struct hashmap *config_cache; + struct config_cache_entry *entry; + struct hashmap_iter iter; + if (!hashmap_initialized) + return; + config_cache = get_config_cache(); + hashmap_iter_init(config_cache, &iter); + while ((entry = hashmap_iter_next(&iter))) { + free(entry->key); + string_list_clear(&entry->value_list, 1); + } + hashmap_free(config_cache, 1); + hashmap_initialized = 0; +} +
The idea was only initialize the hashmap only once, as it is fed by git_config(). I ended up following this approach where the map would be lazily generated whenever a function asked for the map. Is there another way out?