Re: [PATCH 1/5] hashmap: add enum for hashmap free_entries option

From: Karsten Blees <karsten.blees@gmail.com>
Date: Jun 11, 2014, 09:12 UTC
Message-ID: <53981D6A.3090604@gmail.com>
In-Reply-To: <20140610101744.GA23370@t2784.greatnet.de>

Am 10.06.2014 12:17, schrieb Heiko Voigt:

Show 40 quoted lines

> On Fri, Jun 06, 2014 at 07:52:03PM +0200, Karsten Blees wrote:
>> Am 05.06.2014 08:06, schrieb Heiko Voigt:
>>> This allows a reader to immediately know which options can be used and
>>> what this parameter is about.
>>>
>> [...]
>>> -void hashmap_free(struct hashmap *map, int free_entries)
>>> +void hashmap_free(struct hashmap *map, enum hashmap_free_options free_entries)
>> [...]
>>>  
>>> +enum hashmap_free_options {
>>> +	HASHMAP_NO_FREE_ENTRIES = 0,
>>> +	HASHMAP_FREE_ENTRIES = 1,
>>> +};
>>
>> This was meant as a boolean parameter. Would it make sense to have
>>
>> enum boolean {
>> 	false,
>> 	true
>> };
>>
>> or similar in some central place?
> 
> The intention of Jonathans critique here[1] was that you do not see what
> this parameter does on the callsite. I.e.:
> 
> 	hashmap_free(&map, 1);
> 
> compared to
> 
> 	hashmap_free(&map, HASHMAP_FREE_ENTRIES);
> 
> A boolean basically transfers the same information and would not help
> the reader here.
> 
> Cheers Heiko
> 
> [1] http://article.gmane.org/gmane.comp.version-control.git/243917
>

There are languages where you can have e.g. 'hashmap_free(..., free_entries: true)'. In C, however, you do not see what a parameter does at the call site. This is a general language feature, reducing redundancy and keeping it short and concise. IMO there's no reason to treat boolean parameters differently.

Using an enum suggests that there is more to the parameter than a simple yes/no decision, underpinned by naming it '...options' (plural). I find this rather confusing.

Finally, enums share a global namespace, which means long identifiers, provoking additional line breaks and thus reducing readability. Not a problem with hashmap_free per se, but if you do the same for e.g. 'free_util' in string-list.[ch] or 'icase' in name-hash.c, I suspect it'll get pretty ugly.

So please lets not spoil the global namespace with a thousand different names for 0/1. Using enums for >= tristate values and bit flags is fine, but inventing enums for every simple boolean in the system is bound to end in chaos.

Just my 2c Karsten