Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 23, 2014, 23:14 UTC
Message-ID: <xmqqwqc79q1p.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1403518300-23053-3-git-send-email-tanayabh@gmail.com>

Tanay Abhra <tanayabh@gmail.com> writes:

Show 8 quoted lines

> +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?