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

[Heiko Voigt <hvoigt@hvoigt.net>]

On Tue, Jun 17, 2014 at 10:30:35AM +0200, Karsten Blees wrote:
> 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.

If this is such a controversial change for you I will drop this patch in
the next round. I think it would make the callsite more readable without
adding much clutter but I am fine with it either way.