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

[Karsten Blees <karsten.blees@gmail.com>]

Am 12.06.2014 21:12, schrieb Junio C Hamano:
> Karsten Blees <karsten.blees@gmail.com> writes:
> 
>> Am 10.06.2014 12:17, schrieb Heiko Voigt:
>>> 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.
> 
> But given that you are writing in C, is any of that relevant?  We do
> want to keep our call-sites readable and understandable, 

But in C, readable and understandable are opposite goals.
'Understandable' entails long, redundant identifiers, automatically
decreasing readability. The compiler doesn't care about either, so
we could just as well keep the C part short and use plain English
for understandability:

  /* free maps, except file entries (owned by istate->cache) */
  hashmap_free(&istate->name_hash, 0);
  hashmap_free(&istate->dir_hash, 1);

Note that this not only explains what we're doing, but also why.

> and 1 or
> true would not help, unless (1) you are the one who wrote the
> function and know that 1 means free the entries, or (2) the API is
> so widely used and everybody knows what 1 means free the entries.
> 

or (3) you need to check the function declaration or documentation
anyway, to understand what the non-boolean parameters do.

E.g. consider this (from remote.c:1186):

  dst_value = resolve_ref_unsafe(matched_src->name, sha1, 1, &flag);

vs.

  dst_value = resolve_ref_unsafe(matched_src->name, sha1,
                                 RESOLVE_REF_UNSAFE_FOR_READING,
                                 &flag);

That's three lines vs. one, "RESOLVE_REF_UNSAFE_" is completely
redundant with the function name, "FOR_READING" isn't particularly
enlightening either, and you still don't know what the other three
parameters do. IMO this would be much better:

  /* fully resolve matched symref to resolved ref name and sha1 */
  dst_value = resolve_ref_unsafe(matched_src->name, sha1, 1, &flag);

So veterans highly familiar with the code can stick to the C part
without being distracted by unnecessary line breaks and
SHOUTED_IDENTIFIERS, while everyone else may find the explanation
helpful.


As I said, using enums for hashmap_free isn't a problem on its own.
However, the accepted solution for booleans in the git code base
seems to be to use just an int and 0/1.

For consistency, we could of course change string_list*,
resolve_ref*, index_file_exists etc. as well.

...and in turn 'extern int ignore_case' (because it gets passed to
index_file_exists)?

...and in turn all other boolean config variables?

I don't think this would be an improvement, though.